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

refactor: Improved use of type annotations, replace typing by t #986

Merged
merged 17 commits into from
Jan 9, 2024

Conversation

ArneGudermann
Copy link
Contributor

Resolve the typing part of #958

@ArneGudermann ArneGudermann added Priority: Low This issue can be considered with enough idle time. python Pull requests that update Python code syntax labels Dec 11, 2023
@ArneGudermann ArneGudermann added this to the ViUR-core v3.6 milestone Dec 11, 2023
Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

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

Hello @ArneGudermann,
thanks for facing this big issue.

Since Python 3.9 and PEP-585, type annotations do not rely on the typing-module. Therefore, I would like to use this newer syntax consequently.

I would like to generally suggest the following changes:

  • t.Dict replace by dict
  • t.List replace by list
  • t.Union replace by |

This also makes the import of typing pointless in most situations.
What does @sveneberth think about this suggestion?

@sveneberth
Copy link
Member

sveneberth commented Dec 13, 2023

I would like to generally suggest the following changes:

  • t.Dict replace by dict
  • t.List replace by list
  • t.Union replace by |

Yes is good. Use everything that works as a generic, including tuple. In addition, it is then clear that it is prototypes.List and not typing.List.

We can thing also if we prefer str | None over t.Optional[str]. I think t.Optional also expresses that it is really just an argument / option that has to be used and passed if necessary. But technically, of course, it comes down to the same thing.

@phorward
Copy link
Member

Yes is good. Use everything that works as a generic, including tuple. In addition, it is then clear that it is prototypes.List and not typing.List.

Err... should we now use my suggestion, or is typing.Dict, etc. better?

Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

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

Hello @ArneGudermann, thanks for the update.
I really want to merge this quickly.

Can you please check in your code to use set[x] instead of t.Set[x] and to not import t in case it is not used?

Afterwards, we can merge this, to avoid huger merge conflicts in future, as this changes a lot of code.

src/viur/core/bones/record.py Outdated Show resolved Hide resolved
src/viur/core/bones/record.py Outdated Show resolved Hide resolved
src/viur/core/bones/record.py Outdated Show resolved Hide resolved
src/viur/core/bones/password.py Outdated Show resolved Hide resolved
src/viur/core/bones/numeric.py Outdated Show resolved Hide resolved
src/viur/core/bones/string.py Outdated Show resolved Hide resolved
src/viur/core/bones/text.py Outdated Show resolved Hide resolved
src/viur/core/bones/text.py Outdated Show resolved Hide resolved
src/viur/core/config.py Outdated Show resolved Hide resolved
src/viur/core/skeleton.py Outdated Show resolved Hide resolved
@phorward
Copy link
Member

@ArneGudermann please resolve current conflicts.
@sveneberth can you please file your review, too?

@phorward phorward added Priority: High After critical issues are fixed, these should be dealt with before any further issues. and removed Priority: Low This issue can be considered with enough idle time. labels Dec 19, 2023
phorward
phorward previously approved these changes Dec 19, 2023
src/viur/core/email.py Outdated Show resolved Hide resolved
src/viur/core/email.py Outdated Show resolved Hide resolved
@phorward phorward changed the title refactor: Replace typing with t refactor: Improved use of type annotations, replace typing by t Jan 9, 2024
@phorward phorward changed the title refactor: Improved use of type annotations, replace typing by t refactor: Improved use of type annotations Jan 9, 2024
@phorward phorward changed the title refactor: Improved use of type annotations refactor: Improved use of type annotations, replace typing by t Jan 9, 2024
@phorward phorward merged commit 37be9aa into viur-framework:develop Jan 9, 2024
3 checks passed
phorward added a commit that referenced this pull request Jan 9, 2024
phorward added a commit that referenced this pull request Jan 9, 2024
Mostly replaced `typing` by `t`
Resolve the typing part of #958

---------

Co-authored-by: Jan Max Meyer <[email protected]>
Co-authored-by: Sven Eberth <[email protected]>
phorward added a commit that referenced this pull request Jan 9, 2024
Mostly replaced `typing` by `t`
Resolve the typing part of #958

---------

Co-authored-by: Jan Max Meyer <[email protected]>
Co-authored-by: Sven Eberth <[email protected]>
@sveneberth sveneberth linked an issue Jan 11, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High After critical issues are fixed, these should be dealt with before any further issues. python Pull requests that update Python code syntax
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Refactoring typing imports / usage
3 participants