-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
replace pyupgrade+isort+autoflake with ruff #105
Conversation
… isort), skip test_guesses_shed_is_first_party inside worktrees
… handle falsey literals
libcst 1.2 dropped support for 3.8. Resolving it by generating |
ugh that broke 3.12 somehow. Seems like we might actually have to drop 3.8? |
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 again @jakkdl, this is looking great!
(especially the documentation so that ruff
can pick up more of the codemods... extremely valuable and would have sat on my 'todo someday' list ~forever)
# TODO: I guess this required frozenset because of isort compatibility | ||
# but we can probably relax that to an iterable[str] | ||
assert isinstance(first_party_imports, frozenset) |
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 think it was more because I prefer strict validation unless there's a specific reason not to, and there's the 'str
is an iterable of str
' footgun.
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.
ugh, right. I suppose I could change it to Iterable[str]
and assert not isinstance(first_party_imports, str)
?
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.
and then convert it to a frozenset? Sounds good.
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.
It was previously passed to isort
as a frozenset, but now we only use it as a parameter to ruff - where I currently do (the equivalent of) str(list(first_party_imports))
for formatting it. So if we'd only accept a list[str]
we don't need to convert the type... not that it really matters though.
# TODO: isort.place_module is horrendously complicated, but we only use | ||
# a fraction of the functionality. So best approach, if we still need | ||
# the ability to exclude stdlib modules here, is probably to generate a list of | ||
# known stdlib modules - either dynamically or store in a file. | ||
if p.isidentifier() # and place_module(p) != "STDLIB" |
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.
Yeah, having a known list seems fine, and we can just take the union of stdlib modules in all non-EOL Python versions.
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.
Oh, on >3.10 there's https://docs.python.org/3/library/sys.html#sys.stdlib_module_names so only need to generate static lists for 3.8 and 3.9
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.
or well, depends on how thorough we want to be + the level of support for running-version-being-different-from-checked-version
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.
https://pypi.org/project/stdlibs/ sounds pretty great if we want to be maximum thorough
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.
Let's just use that on 3.10+, and vendor the 3.10 list for older versions 👍
Oh, can just add For funsies I also tried running 3.13 locally, but encountered HypothesisWorks/hypothesis#3916 Mostly unrelated to this PR:Looking at https://github.com/Zac-HD/shed/actions/runs/8175453287/job/22352606988#step:5:75 says
Not that it matters much here (I think? kinda weird error...), but is it possible to get at that patch in any way when it's run in CI? If not, does hypothesis have a flag for printing that patch in a different way to the log so a user could make use of it? |
Added/modified comments
…py' instead of 'python'
The CI check previously failed because |
If you've configured artifact upload for that directory, yes. It's useful to print them though, because if there's a printable example we already show them - in this case here with |
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 think this is a nice improvement over the status quo, so merging now.
If you want to make a follow-up PR with the stdlib list and optionally the iterable/frozenset handling, plus a new version number, I'll release shortly 🎉
Take 3 after
master...use-ruff and #102
fixes #101
Notes:
pass
also removes unnecessary...
, hence change inpattern-matching.txt
.remaining TODOs:
ruff format
(could leave that for a separate PR)