Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data API data model extension #624

Merged
merged 8 commits into from
Mar 20, 2023
Merged

Data API data model extension #624

merged 8 commits into from
Mar 20, 2023

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.

None yet

3 participants