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.

Sign up for GitHub

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

Jump to bottom

Fix type subscript on older python versions #15090

Merged
merged 1 commit into from
Dec 21, 2024
Merged

Fix type subscript on older python versions #15090

merged 1 commit into from
Dec 21, 2024

Conversation

aaron-skydio
Copy link
Contributor

@aaron-skydio aaron-skydio commented Dec 21, 2024 •
edited
Loading

Summary

Python versions before Python 3.9 don't have subscriptable containers like list, which is being used in the annotation of get_last_three_path_parts here. I believe Ruff supports Python 3.7 and Python 3.8? In that case this needs to either use the container from the typing module, or use deferred annotations with from __future__ import annotations like I'm proposing here

Introduced by 60a2dc5

Test Plan

Currently the code path in find_ruff_bin that defines get_last_three_path_parts doesn't work on Python 3.8 or 3.9, it throws this:

>>> ruff.__main__.find_ruff_bin()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/aaron/miniconda3/envs/symforce/lib/python3.8/site-packages/ruff/__main__.py", line 77, in find_ruff_bin
    raise FileNotFoundError(scripts_path)
FileNotFoundError: /home/aaron/miniconda3/envs/symforce/bin/ruff

With this change it does work

@aaron-skydio
Fix type subscript on older python versions
949275b 
Python versions before Python 3.9 don't have subscriptable containers like `list`, which is being used in the annotation of `get_last_three_path_parts` here.  I believe Ruff supports Python 3.7 and Python 3.8?  In that case this needs to either use the container from the `typing` module, or use deferred annotations with `from __future__ import annotations` like I'm proposing here
@github-actions GitHub Actions
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser
Copy link
Member

MichaReiser commented Dec 21, 2024 •
edited
Loading

Thanks for PRing this fix

@charliermarsh would you mind taking a look at this PR? I normally would ask Alex but he's out and I'm probably not the best person to review this change :)

@MichaReiser MichaReiser added the bug Something isn't working label Dec 21, 2024
@MichaReiser
Copy link
Member

I guess that's actually fine. Even I understand why this is needed with a summary as good as this one. Thank you

@MichaReiser MichaReiser merged commit fd4bea5 into astral-sh:main Dec 21, 2024
21 checks passed
@charliermarsh
Copy link
Member

Yeah this looks correct to me.

@aaron-skydio aaron-skydio deleted the patch-1 branch December 22, 2024 19:38
@BrewTestBot BrewTestBot mentioned this pull request Jan 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aaron-skydio @MichaReiser @charliermarsh