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

style[eve]: remove unused imports and fix typos #1748

Merged
merged 7 commits into from
Dec 2, 2024

Conversation

romanc
Copy link
Contributor

@romanc romanc commented Nov 26, 2024

Description

Small cleanup PR in the eve framework:

  • Removes a stale .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.
  • Remove a couple of stale includes. The language server marked them as unused and since tests still pass, I guess we really don't need them anymore.
  • Fixed a couple of typos in comments
  • Fixed two typos in the github PR template

Requirements

  • All fixes and/or new features come with corresponding tests.
    N/A: neither fixes nor new features
  • Important design decisions have been documented in the appropriate ADR inside the docs/development/ADRs/ folder.
    N/A

Comment on lines -92 to -100
"# datamodels" "Coerced",
"DataModel",
"FrozenModel",
"GenericDataModel",
"Unchecked",
"concretize",
"datamodel",
"field",
"frozenmodel",
Copy link
Contributor Author

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.

@romanc romanc force-pushed the romanc/cleanup-eve branch from 3161005 to fbd4461 Compare November 26, 2024 15:16
Copy link
Contributor

@egparedes egparedes left a 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.

src/gt4py/eve/__init__.py Outdated Show resolved Hide resolved
@romanc
Copy link
Contributor Author

romanc commented Nov 26, 2024

Thanks for the cleanup. I have just a question and a comment.

Thanks for the inline comment: missed that 👍

What was your question?

@romanc romanc marked this pull request as ready for review November 26, 2024 15:23
@romanc
Copy link
Contributor Author

romanc commented Nov 27, 2024

@twicki @FlorianDeconinck FYI: the great cleanup continues ...

@romanc romanc requested a review from egparedes November 27, 2024 16:20
@romanc
Copy link
Contributor Author

romanc commented Dec 2, 2024

ping @egparedes

Copy link
Contributor

@egparedes egparedes left a 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.

Comment on lines 80 to 83
from contextlib import (
AbstractAsyncContextManager as AsyncContextManager,
AbstractContextManager as ContextManager,
)
Copy link
Contributor

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.

Copy link
Contributor Author

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 👍

Roman Cattaneo and others added 7 commits December 2, 2024 12:01
@romanc romanc force-pushed the romanc/cleanup-eve branch from cc52a5e to 41127d9 Compare December 2, 2024 11:05
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

LGTM

@egparedes egparedes merged commit 6f49699 into GridTools:main Dec 2, 2024
43 checks passed
@romanc romanc deleted the romanc/cleanup-eve branch December 2, 2024 13:30
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.

2 participants