Skip to content

Conversation

@emanuel-schmid
Copy link
Collaborator

Changes proposed in this PR:

  • Add optional List properties to DataTypeInfo class: key_reference and version_notes. They are necessary in order to cope with the soon to be updated data_type Json Schema.
  • Add trailing '/' for various urls, that's more consistent with the servers' url definitions and allows for testing on localhost
  • Use a regular expression for extracting host and port from url (also for testing on localhost)

This PR fixes #

PR Author Checklist

PR Reviewer Checklist

@emanuel-schmid emanuel-schmid marked this pull request as draft January 17, 2023 16:52
@zeliest
Copy link
Collaborator

zeliest commented Jan 20, 2023

I tried to run the test, tutorials and some of my scripts using the API. Everything runs fine. version_notes is to specify what has been changed in each version then? what about key_reference?

Comment on lines 261 to 268
urlpat = r"http(s?)://([^:/]+)(:\d+)?(/|$)"
match = re.match(urlpat, url)
if match:
host = match.group(2)
port = int(match.group(3)[1:]) if match.group(3) else 443 if match.group(1) else 80
else:
raise ValueError(f'URL not as expected, cannot figure out host and port from "{url}"')
return host, port
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use a regex, there is a function to extract url components: urllib.parse.urlsplit

Suggested change
urlpat = r"http(s?)://([^:/]+)(:\d+)?(/|$)"
match = re.match(urlpat, url)
if match:
host = match.group(2)
port = int(match.group(3)[1:]) if match.group(3) else 443 if match.group(1) else 80
else:
raise ValueError(f'URL not as expected, cannot figure out host and port from "{url}"')
return host, port
url_split = urlsplit(url)
port = url_split.port
if port is None:
port = 443 if url_split.scheme == "https" else 80
return url_split.netloc, port

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool 😎

# Conflicts:
#	climada/util/api_client.py
@emanuel-schmid emanuel-schmid marked this pull request as ready for review March 15, 2023 12:54
- better integration of data_type versions and references
- data_type properties filtered by extant dataset poperties
@emanuel-schmid emanuel-schmid merged commit 86f4468 into develop Mar 20, 2023
@emanuel-schmid emanuel-schmid deleted the feature/db_extension branch March 20, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants