Closed snejus closed 1 month ago
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.
Nice! I'll have a look at this during the weekend. (In fact, I've also been working on this myself, let's see whether we came to the same conclusions.)
Nice! I'll have a look at this during the weekend. (In fact, I've also been working on this myself, let's see whether we came to the same conclusions.)
I'm intrigued!
Do we still support Python 3.7 @Serene-Arc @wisp3rwind?
Do we still support Python 3.7 @Serene-Arc @wisp3rwind?
According to grep
, we state that we support 3.7+. However, given that Python 3.7 has been EOL for almost a year, I guess we should drop it as soon as there's any new feature from 3.8 that we'd like to use.
Do we still support Python 3.7 @Serene-Arc @wisp3rwind?
According to
grep
, we state that we support 3.7+. However, given that Python 3.7 has been EOL for almost a year, I guess we should drop it as soon as there's any new feature from 3.8 that we'd like to use.
I'd be more than happy to drop it immediately then! I attempted to use cached_property
here and the walrus operator but failed :(
Do we still support Python 3.7 @Serene-Arc @wisp3rwind?
According to
grep
, we state that we support 3.7+. However, given that Python 3.7 has been EOL for almost a year, I guess we should drop it as soon as there's any new feature from 3.8 that we'd like to use.I'd be more than happy to drop it immediately then! I attempted to use
cached_property
here and the walrus operator but failed :(
Ok, than maybe let's not do it for this PR (it's not really required I suppose), but open a new issue to give people a chance to raise complaints about dropping Python 3.7.
So, before I start looking at this in depth: Would you mind splitting into 3 PRs, namely:
(Or at least separate out the latter.) That would help a lot with reviewing, because the first two are pretty safe changes since they're not really changing any code. We should be able to merge them quickly. The latter might warrant more scrutiny.
So, before I start looking at this in depth: Would you mind splitting into 3 PRs, namely:
- the two mypy config commits
- everything except the legalize_path changes
- the legalize_path changes
(Or at least separate out the latter.) That would help a lot with reviewing, because the first two are pretty safe changes since they're not really changing any code. We should be able to merge them quickly. The latter might warrant more scrutiny.
Of course!
@snejus could you also open another PR with the mypy config changes? Thanks!
Superseded by #5232 #5223, #5224.
Description
I noticed that two thirds of all typing issues in the codebase came from
beets.util.__init__
module, so I went ahead and addressed them.Before
After
Context for the
_legalize_path
/_legalize_stage
updateMypy was not happy here because
_legalize_stage
function implementation concatenatespath
andextension
parameters, implying that their types need to match.You can see that initially
path
parameter was defined as astr
whileextension
wasbytes
.In reality, depending on the
fragment
parameter value,extension
was sometimes provided as astr
and sometimes asbytes
. The same parameter decided whetherpath
gets converted intobytes
within_legalize_stage
implementation. No surprise that mypy was confused here._legalize_stage
is only used withinItem.destination
method implementation which is wherefragment
is defined. I determined that thefragment
parameter controls the form of the output path:fragment=False
returned absolute path as bytes (default)fragment=True
returned path relative to the library directory as str.Given the above, the change
Renames
fragment
parameter torelative_to_libdir
for clarityMakes
Item.destination
to return the same type in both cases. I pickedbytes
since that's the type that majority of the code using this method expects.I converted the output path to
str
for the code that has been expecting a string there.Decouples
_legalize_stage
and_legalize_path
implementations from therelative_to_libdir
. The logic now usesstr
type only.