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

gh-355: consistent use of typing and collections.abc #356

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

paddyroddy
Copy link
Member

@paddyroddy paddyroddy commented Oct 14, 2024

Fixes #355, simplifying #308, xref #350. Change all import of typing/collections.abc to import <x> rather than from <x> import syntax. This is useful because it is straightforward to see at a glance where the import is coming from. Further, we have both numpy.random.Generator and collections.abc.Generator in use - so it separates them. I've also moved everything out of the TYPE_CHECKING block, except from where ruff says we should.

Fixes #355, simplifying #308. Change all import of
`typing`/`collections.abc` to `import <x>` rather than
`from <x> import` syntax. This is useful because it is straightforward
to see at a glance where the `import` is coming from. Further, we have
both `numpy.random.Generator` and `collections.abc.Generator` in use -
so it separates them. I've also moved everything out of the
`TYPE_CHECKING` block, except from where `ruff` says we should.
@paddyroddy paddyroddy added enhancement New feature or request maintenance Maintenance: refactoring, typos, etc. labels Oct 14, 2024
@paddyroddy paddyroddy self-assigned this Oct 14, 2024
"if TYPE_CHECKING:",
"if typing.TYPE_CHECKING:",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is important, we might potentially want to add both in case someone doesn't use the format this PR is trying to enforce

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @paddyroddy! Looks good!

@paddyroddy paddyroddy merged commit c15af39 into main Oct 14, 2024
19 checks passed
@paddyroddy paddyroddy deleted the paddy/issue-355 branch October 14, 2024 14:24
@paddyroddy paddyroddy mentioned this pull request Oct 14, 2024
3 tasks
@ntessore
Copy link
Collaborator

I really don't like this change, as it goes against established typing practice (e.g., https://typing.readthedocs.io).

@paddyroddy
Copy link
Member Author

paddyroddy commented Oct 15, 2024

I really don't like this change, as it goes against established typing practice (e.g., typing.readthedocs.io).

It is how I've done it in all my work with ARC. Explicit imports make it so much easier to know where something comes from. However, this can be undone - but preferably after #359 #308

@paddyroddy
Copy link
Member Author

It also clutters the namespace https://softwareengineering.stackexchange.com/a/187432/320391

@ntessore
Copy link
Collaborator

It also clutters the namespace https://softwareengineering.stackexchange.com/a/187432/320391

Only if the imports are not guarded by the future import and if TYPE_CHECKING:. I usually don't have much, if any, of the static type present at runtime.

@paddyroddy
Copy link
Member Author

Have raised #361

paddyroddy added a commit that referenced this pull request Oct 15, 2024
Adding `mypy` to a pre-existing codebase is never easy. I initially
attempted this in #308, but have since split this up into separate
issues:
- [x] #347
- [x] #356
- [ ] #358

In this PR, `mypy` is added but in order for CI to pass, the `# type:
ignore[]` syntax is used throughout. I didn't want to tackle these here
too (see #308) as it gets quite messy.

One thing I have done (following #356) is change every empty
`npt.NDArray` to `npt.NDArray[typing.Any]` (see #330), as this actually
results in fewer errors than leaving them all blank. Ideally, we'd like
to fill in as many of the `typing.Any` as possible - they're a bit
useless by themselves. However, that is not the priority for now. Plus,
I expect typing to change when #67 is tackled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request maintenance Maintenance: refactoring, typos, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tidy up typing
3 participants