Fix types in beets/util/__init__.py

[snejus]

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

🐶 mypy beets beetsplug test | tail -1
Found 91 errors in 16 files (checked 188 source files)

🐶 mypy beets beetsplug test --no-error-summary | sed 's/:.*//' | sort | uniq -c | sort -nr
      64 beets/util/__init__.py
      9 test/test_ui_importer.py
      7 beets/library.py
      6 test/plugins/test_player.py
      5 test/plugins/test_albumtypes.py
      4 beetsplug/playlist.py
      4 beetsplug/lastgenre/__init__.py
      1 test/test_plugins.py
      1 test/test_config_command.py
      1 test/plugins/test_mpdstats.py
      1 test/plugins/test_mbsubmit.py
      1 test/plugins/test_edit.py
      1 beetsplug/the.py
      1 beets/plugins.py
      1 beetsplug/fetchart.py
      1 beetsplug/edit.py
      1 beets/logging.py
      1 beets/autotag/hooks.py

After

🐶 mypy beets beetsplug test | tail -1
Found 46 errors in 14 files (checked 188 source files)

🐶 mypy beets beetsplug test --no-error-summary | sed 's/:.*//' | sort | uniq -c | sort -nr
     15 beets/library.py
      9 test/test_ui_importer.py
      6 test/plugins/test_player.py
      5 test/plugins/test_albumtypes.py
      4 beetsplug/playlist.py
      4 beetsplug/lastgenre/__init__.py
      1 test/test_plugins.py
      1 test/test_config_command.py
      1 test/plugins/test_mbsubmit.py
      1 test/plugins/test_edit.py
      1 beetsplug/the.py
      1 beets/plugins.py
      1 beetsplug/fetchart.py
      1 beetsplug/edit.py
      1 beets/logging.py
      1 beets/autotag/hooks.py

Context for the _legalize_path / _legalize_stage update

Mypy was not happy here because _legalize_stage function implementation concatenates path and extension parameters, implying that their types need to match.

You can see that initially path parameter was defined as a str while extension was bytes.

In reality, depending on the fragment parameter value, extension was sometimes provided as a str and sometimes as bytes. The same parameter decided whether path gets converted into bytes within _legalize_stage implementation. No surprise that mypy was confused here.

_legalize_stage is only used within Item.destination method implementation which is where fragment is defined. I determined that the fragment 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

1. Renames fragment parameter to relative_to_libdir for clarity

2. Makes Item.destination to return the same type in both cases. I picked bytes 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.

3. Decouples _legalize_stage and _legalize_path implementations from the relative_to_libdir. The logic now uses str type only.

[github-actions]

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.

[wisp3rwind]

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.)

[snejus]

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!

[snejus]

Do we still support Python 3.7 @Serene-Arc @wisp3rwind?

[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.

[snejus]

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 :(

[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.

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.

[wisp3rwind]

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.

[wisp3rwind]

This is still required.

[snejus]

Yup I accidentally removed it when I reversed my cached_property import, assuming that nothing else needs this import.

[snejus]

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!

[wisp3rwind]

@snejus could you also open another PR with the mypy config changes? Thanks!

[wisp3rwind]

Superseded by #5232 #5223, #5224.