Skip to content

Conversation

@ebrahimebrahim
Copy link
Collaborator

  • Remove the exception to this linting rule
  • Allow ruff to make bulk changes to satisfy the new rule

Making the linter more strict on this makes it clear how to fix type annotation complaints, avoiding some confusing situations

@ebrahimebrahim ebrahimebrahim force-pushed the remove_exception_for_future_annotations_import branch from 0ad85d1 to 2afdc81 Compare February 28, 2025 21:29
Copy link
Contributor

@arhowe00 arhowe00 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, I'd recommend modifying the commit message to also include the fact that ruff removed Optional[•] to use • | None, as I don't think it's related to __future__ annotations, but it's fine either way and ready to rebase and merge.

@ebrahimebrahim ebrahimebrahim force-pushed the remove_exception_for_future_annotations_import branch from 2afdc81 to 31848dd Compare March 4, 2025 01:03
@ebrahimebrahim
Copy link
Collaborator Author

as I don't think it's related to __future__ annotations, but it's fine either way and ready to rebase and merge.

It is related: only once future annotations become available can the | notation be used in python 3.9. This is why ruff then insists on doing it. standardizing will just help cause less confusion in the future

@ebrahimebrahim ebrahimebrahim requested a review from arhowe00 March 4, 2025 01:04
Once __future__ annotations become available the `|` notation can
be used in python 3.9. So ruff was here allowed to standardize
type annotations based on that. This will help reduce confusion about
linter complaints in the future.
@ebrahimebrahim ebrahimebrahim force-pushed the remove_exception_for_future_annotations_import branch from 31848dd to 6d8d67d Compare March 4, 2025 16:41
@arhowe00 arhowe00 enabled auto-merge (rebase) March 4, 2025 16:43
@arhowe00 arhowe00 merged commit e84abdf into main Mar 4, 2025
9 checks passed
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