Fix most types in beets.util.__init__

snejus commented 5 months ago

Description

This PR is Part 1 of the work #5215 that fixes typing issues in beets.util.__init__ module.

It addresses simple-to-fix / most of the issues in this module.

github-actions[bot] commented 5 months 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.

wisp3rwind commented 4 months ago

I left a few more comments. The one thing that remains and where we seem to disagree is this:

Ideally, I'd imagine these functions should accept the same types that the original ones do. In this case it's os.walk, which accepts any path-like str or bytes and returns the same type that it received.

On the other hand, this PR was about fixing types rather than adjusting the implementation, so I focused on underlining what the current implement does and handles to make life easier to the person who will need to refactor this module later.

I'd be happy with making these functions to handle bytes only if

We explicitly handle bytes only - remove the code that casts str to bytes from the beginning of the implementation Rename the functions to make the only-bytes-accepted-here requirement clear, since the equivalents in os (normpath, samefile, ...) handle str and bytes types. Therefore, to not confuse people used to the builtins, we'd want to name these functions accordingly I suppose.

I guess there are two philosophies here:

Add typings according to anything the code accepts currently. This significantly reduces how much the type checker can help us to verify that our current path handling is correct, but might be the quicker path to a clean mypy run.
Add typings according to what is vaguely defined to be the intended usage (e.g. always call syspath with bytes). There may be some code around that would fail to type check due to this. I'd argue that this is actually a good thing, and probably indicates that the calling code needs to be fixed. In this interpretation, more lenient implementation (i.e. no assert isinstance(path, bytes) is a form of defensive programming.

I don't think we should start adding such assert statements immediately: This is too likely to cause issues. If we want to go this way, I think the steps should be

Type the entire beets code base and fix any callers
Add a warning where the assert should ultimately be
Do a release, and mention in the release notes that plugin authors need to anticipate this change
swap warnings for asserts

I'm not sure what's the best way to proceed here. I would very much like to see 2. happen eventually. A broader roadmap could be:

Type the entire beets code base, erring on the more lenient side.
Prepare an updated version of https://beets.io/blog/paths.html, this should probably live in this repository rather than a blog. This should have clear guidance on what to do when
- passing paths to os.path. and similar APIs
- reading/writing paths or path fragments from/to files (e.g. playlist files)
- passing paths to subprocesses
- storing paths in the database
- reading paths or path fragments from the configuration
- ...
Tighten up our typings accordingly and add warnings where the ultimately we would want assertions.
Do a release, and mention in the release notes that plugin authors need to anticipate this change
swap warnings for asserts
At this point, I think we'd be in a good position to revisit a conversion to pathlib. We should start by updating our design document from the second point.

wisp3rwind commented 4 months ago

snejus commented 4 months ago

Weird stuff: as you've seen my change made that single test fail on Windows, see this build 22e2a3c for example

FAILED test/plugins/test_web.py::WebPluginTest::test_get_item_file - Attribut...
===== 1 failed, 1639 passed, 123 skipped, 9 xfailed in 199.92s (0:03:19) ======

Now in the next commit https://github.com/beetbox/beets/pull/5223/commits/abd8447218c6e340a9f672b140e4c0fcd71318c2 I reverted my change and I got bombarded with this

FAILED test/plugins/test_convert.py::ImportConvertTest::test_delete_originals
FAILED test/plugins/test_convert.py::ImportConvertTest::test_import_converted
FAILED test/plugins/test_convert.py::ConvertCliTest::test_convert - TypeError...
FAILED test/plugins/test_convert.py::ConvertCliTest::test_convert_keep_new - ...
FAILED test/plugins/test_convert.py::ConvertCliTest::test_convert_with_auto_confirmation
FAILED test/plugins/test_convert.py::ConvertCliTest::test_embed_album_art - T...
FAILED test/plugins/test_convert.py::ConvertCliTest::test_format_option - Typ...
FAILED test/plugins/test_convert.py::ConvertCliTest::test_no_transcode_when_maxbr_set_high_and_different_formats
FAILED test/plugins/test_convert.py::ConvertCliTest::test_playlist - TypeErro...
FAILED test/plugins/test_convert.py::ConvertCliTest::test_transcode_when_maxbr_set_low_and_different_formats
FAILED test/plugins/test_convert.py::ConvertCliTest::test_transcode_when_maxbr_set_low_and_same_formats
FAILED test/plugins/test_convert.py::ConvertCliTest::test_transcode_when_maxbr_set_to_none_and_different_formats
FAILED test/plugins/test_convert.py::NeverConvertLossyFilesTest::test_transcode_from_lossless
FAILED test/plugins/test_convert.py::NeverConvertLossyFilesTest::test_transcode_from_lossy
===== 14 failed, 1628 passed, 121 skipped, 9 xfailed in 194.90s (0:03:14) =====

https://github.com/beetbox/beets/assets/16212750/56e3cb3a-daca-41dc-b8bb-f5300b953173

snejus commented 2 weeks ago

@wisp3rwind as you suggested I used bytes wherever possible and resolved the typing issues without having to use AnyStr everywhere.

I'd be happy to move this forward if you have time to have another look!

snejus commented 2 weeks ago

Otherwise, it seems that there have been some changes around syspath and the bytes vs str vs Path question. I don't think I'm qualified to comment on the correctness of related changes here, I'm definitely out of the loop on what the correct usage of these types would currently be.

@wisp3rwind note that syspath implementation has not been adjusted:

wisp3rwind commented 2 weeks ago

Otherwise, it seems that there have been some changes around syspath and the bytes vs str vs Path question. I don't think I'm qualified to comment on the correctness of related changes here, I'm definitely out of the loop on what the correct usage of these types would currently be.

@wisp3rwind note that syspath implementation has not been adjusted:

Not here; what I meant is that it has changed recently (in 01c24bb3a72760cd82c3f3417b56c5796ac55077), at a time when I've already been following beets development less closely.

snejus commented 2 weeks ago

Otherwise, it seems that there have been some changes around syspath and the bytes vs str vs Path question. I don't think I'm qualified to comment on the correctness of related changes here, I'm definitely out of the loop on what the correct usage of these types would currently be.

@wisp3rwind note that syspath implementation has not been adjusted:

Not here; what I meant is that it has changed recently (in 01c24bb), at a time when I've already been following beets development less closely.

Ah I see!

That wasn't a very significant change to be honest: I added support for the Path type to syspath and bytestring_path, thus the change involved making sure that the input is cast appropriately.

snejus commented 2 weeks ago

Is there anything else that needs addressing here @wisp3rwind?

wisp3rwind commented 1 week ago

Is there anything else that needs addressing here @wisp3rwind?

Looks fine!

snejus commented 1 week ago

Cool, will merge it in once you've approved it 🙂

beetbox / beets