-
Notifications
You must be signed in to change notification settings - Fork 49
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
style[eve]: remove unused imports and fix typos #1748
Conversation
"# datamodels" "Coerced", | ||
"DataModel", | ||
"FrozenModel", | ||
"GenericDataModel", | ||
"Unchecked", | ||
"concretize", | ||
"datamodel", | ||
"field", | ||
"frozenmodel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are duplicated in the lines to follow.
3161005
to
fbd4461
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup. I have just a question and a comment.
Thanks for the inline comment: missed that 👍 What was your question? |
@twicki @FlorianDeconinck FYI: the great cleanup continues ... |
ping @egparedes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for the late reply, I somehow forgot to add my question before submitting my previous review. This was the missing question.
src/gt4py/eve/extended_typing.py
Outdated
from contextlib import ( | ||
AbstractAsyncContextManager as AsyncContextManager, | ||
AbstractContextManager as ContextManager, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are those removed? IIRC those imports are here just to mimic the deprecated aliases in typing
for older python versions. Once we drop support for 3.8 and 3.9 (hopefully in a matter of weeks) we should delete all these aliases, but until then I think we should keep them for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed them because the vscode language server for python marked them as unused (unlike others in that category). Since 3.8 and 3.9 versions of tests still pass, they might actually be unused. However, I didn't see that they are part of a code block guarded for 3.9 only. If the plan is to drop support for 3.8 and 3.9 in the next weeks, I agree, that there's no need to change anything here since we'll just get rid of the whole block with that PR. I'll revert my changes here 👍
Remove extra quote Co-authored-by: Enrique González Paredes <[email protected]>
To be dealt with when dropping support for python 3.8 and 3.9
cc52a5e
to
41127d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Small cleanup PR in the eve framework:
.gitignore
file. As far as I understood from the git history, earlier versions of this codebase had many.gitignore
files in many places. Looks like this one is a leftover from a previous time.Requirements
N/A: neither fixes nor new features
N/A