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

Feature/update typing #790

Merged
merged 4 commits into from
Sep 6, 2023
Merged

Feature/update typing #790

merged 4 commits into from
Sep 6, 2023

Conversation

gimppa
Copy link
Contributor

@gimppa gimppa commented Aug 29, 2023

Description

Updated type annotations all around. Mostly changed dict and list type annotations to be Dict[something] and List[something].

Related issues

Fixes #789 github workflows

Type of change

  • New feature (non-breaking change which adds functionality)

Testing

  • Tests do not apply

@gimppa gimppa self-assigned this Aug 29, 2023
@gimppa gimppa requested review from csc-jm and csc-felipe August 29, 2023 06:45
@csc-felipe csc-felipe mentioned this pull request Aug 29, 2023
2 tasks
@csc-felipe
Copy link
Contributor

csc-felipe commented Aug 29, 2023

Why should we use Dict and List instead of dict and list in Python 3.11? We can use dict and list directly since Python 3.9.

Which makes me think that perhaps we should set python_version for mypy.

@gimppa
Copy link
Contributor Author

gimppa commented Aug 29, 2023

There was a mix of dict and typing.Dict used (and list / typing.List). As far as I understand, they can be used interchangeably, so I just went with the other.

csc-jm
csc-jm previously requested changes Aug 31, 2023
Copy link
Collaborator

@csc-jm csc-jm left a comment

Choose a reason for hiding this comment

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

I think what @csc-felipe commented is a good point. If we're updating the typing, we might as well do it with the current best practice while we're at it (as long as it's not an unnecessarily long task). So while the built-in typing class still exists, it's been deprecated by now as proposed in PEP 585. So you would have list, dict etc. or then collections.abc.Callable and collections.abc.Iterator.

However, it seems the types Any, Optional, TypedDict and TypeVar which we still have in our code (at least I found in this PR) are still to be imported from typing 🤔

metadata_backend/api/handlers/rems_proxy.py Outdated Show resolved Hide resolved
metadata_backend/api/handlers/restapi.py Outdated Show resolved Hide resolved
@gimppa gimppa requested a review from csc-jm September 5, 2023 14:15
Copy link
Contributor

@csc-felipe csc-felipe left a comment

Choose a reason for hiding this comment

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

Looks good :)

@csc-felipe
Copy link
Contributor

Approved, but you still have to click on the three dots > dismiss review from Joonatan's requested changes.

@gimppa gimppa dismissed csc-jm’s stale review September 6, 2023 06:58

Change implemented

Copy link
Collaborator

@csc-jm csc-jm left a comment

Choose a reason for hiding this comment

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

Now everything looks good to me as well :P

@csc-jm csc-jm merged commit 58cbdff into develop Sep 6, 2023
11 checks passed
@csc-jm csc-jm deleted the feature/update_typing branch September 6, 2023 15:08
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.

3 participants