Drop Python 3.6, some Python 2 + Python <= 3.6 cleanup by wisp3rwind · Pull Request #4374 · beetbox/beets

wisp3rwind · 2022-06-14T20:30:09Z

Description

Fixes #4372; best reviewed commit-by-commit. Don't merge this just yet, I want to review the case_sensitive change again (please do so, too!).

sampsyo

Finally had a chance to read though everything and think through it all! This is really helpful; it's wonderful to excise some of these old and sad workarounds. Here are a few scattered comments. I am pretty convinced of the case_sensitive changes in particular.

…__str__

when symlinks aren't supported, recent python will only raise NotImplementedError (or might even support symlinks), ending up in the other except arm

These didn't match the code anymore (which had already been changes manually or automatically as part of the 2->3 transition).

…rsions

We still need these custom Logger modifications, but the precise reasoning as given in the comments didn't quite apply anymore. Also, `stack_info` and `stacklevel` arguments have been added in recent Python, which we now support.

(at least, more readable to me)

This was a helper for situations when Python 2 and 3 APIs returned bytes and unicode, respectively. In these situation, we should nowadays know which of the two we receive, so there's no need to wrap & hide the `bytes.decode()` anymore (when it is still required). Detailed justification: beets/ui/__init__.py: - command line options are always parsed to str beets/ui/commands.py: - confuse's config.dump always returns str - open(...) defaults to text mode, read()ing str beetsplug/keyfinder.py: - ... beetsplug/web/__init__.py: - internally, paths are always bytestrings - additionally, I took the liberty to slighlty re-arrange the code: it makes sense to split off the basename first, since we're only interested in the unicode conversion of that part. test/helper.py: - capture_stdout() gives a StringIO, which yields str test/test_ui.py: - self.io, from _common.TestCase, ultimately contains a _common.DummyOut, which appears to be dealing with str (cf. DummyOut.get)

- move tests for case_sensitive to test_util.py, since this is not really the concern of PathQueryTest - removes part of the tests, since the tests that patch os.path.samefile and os.path.exists are super brittle since they test the implementation rather than the functionality of case_sensitive(). This is a prepartory step for actually changing the implementation, which would otherwise break the tests in a confusing way...

- samefile exists on all platforms for recent python - don't rely on monkey-patching os/os.path and on specifics on the implementation: as a result of doing so, the tests start failing in obscure ways as soon as the implementation (and its usage of os.path.exists and os.path.samefile) is changed

normpath already applies bytestring_path() to its output

- some cleanup for recent Python which has samefile() on Windows - also, fix this function by only upper-/lower-casing one path component at a time. It is unclear to me how this could have ever worked.

wisp3rwind · 2023-02-12T15:33:17Z

Ok, this should finally be ready to go: The case_sensitive refactor triggered test failures since 1) the tests were monkey-patching things in a very brittle way, 2) PathQuerys were actualyl incorrect due to inconsistent SQL and pure-Python matching. Those were a major hassle to debug; the fix is hopefully reasonably simple to understand. Would be happy about another pair of eyes on those changes.

(This PR is still best studied commit-by-commit, which are well-organized, whereas the scope of the entire PR has grown a lot...)

sampsyo

I've gone through this again commit by commit, and I've found nothing to be concerned about. I share your fears about some of the path-query stuff, which is really quite mind-bending at times (that case_sensitive function is an incredible pile of hacks unto itself).

This seems good to go! Care to hit the green button?

sampsyo · 2023-02-19T04:58:48Z

+        if cls.force_implicit_query_detection:
+            return True


This is a pretty good idea. It's a sort of ugly thing to need to do, but it's much less mysterious than the stdlib mocking we were resorting to before.

Yeah, I don't really like this flag either, but at least it's obvious what's going on.

sampsyo · 2023-02-19T05:00:13Z

+            self.assertFalse(is_path_query('bar'))
        finally:
-            self.patcher_exists.start()
+            os.chdir(cur_dir)


This is some very tricky stuff to get right! Very nice work here. It all looks right to me, though.

sampsyo · 2023-02-19T05:03:32Z

+            # lowercased.
+            # This also ensures that the `match()` method below and the SQL
+            # from `col_clause()` do the same thing.
+            path = path.lower()


Aha, so we need to lower-case the entire normalized path: not just the part the user actually typed as the query pattern. This makes sense, and it is one of those situations where I now wonder how this ever worked before.

JOJ0 · 2023-02-19T10:25:09Z

❤️

wisp3rwind force-pushed the pr_drop_old_python branch 4 times, most recently from a4bd51b to acc2cf4 Compare June 15, 2022 10:35

sampsyo mentioned this pull request Jun 24, 2022

Drop Python 3.6 #4372

Closed

sampsyo reviewed Jun 27, 2022

View reviewed changes

Comment thread beets/library.py Outdated

Comment thread beets/logging.py

Comment thread beetsplug/web/__init__.py

Comment thread beets/library.py

Comment thread beets/util/__init__.py

Comment thread beets/util/__init__.py

wisp3rwind force-pushed the pr_drop_old_python branch 7 times, most recently from 04760e4 to 1d06715 Compare September 25, 2022 15:49

wisp3rwind mentioned this pull request Oct 2, 2022

replaygain: Fix error handling for parallel runs #4506

Merged

wisp3rwind force-pushed the pr_drop_old_python branch 2 times, most recently from c9c4329 to f7871c6 Compare October 16, 2022 07:48

wisp3rwind added 3 commits December 18, 2022 09:20

docs: remove unused link target, update links to python docs

f419543

drop Python 3.6: docs, a few safe simplifications

bd09cc9

drop old Python: simplify datetime usage

6326986

wisp3rwind force-pushed the pr_drop_old_python branch 6 times, most recently from 20f842c to 17f8479 Compare December 18, 2022 14:35

wisp3rwind added 5 commits December 24, 2022 13:07

drop old Python: rm python 2 leftovers in library.FileOperationError.…

5cdb0c5

…__str__

drop old Python: don't handle obsolte exception in keyfinder plugin

67f0c73

drop old Python: remove obsolete custom string formatter in hooks plugin

22826c6

drop old Python: remove obsolete exception handling for Windows symlinks

c816b29

when symlinks aren't supported, recent python will only raise NotImplementedError (or might even support symlinks), ending up in the other except arm

drop old Python: update a bunch of comments, removing Python 2 details

9f58673

These didn't match the code anymore (which had already been changes manually or automatically as part of the 2->3 transition).

wisp3rwind added 4 commits December 24, 2022 13:09

remove old Python: in logging, rm workarounds for broken string conve…

7c2aa02

…rsions

web: slight refactor to make path handling more readable

3510e63

(at least, more readable to me)

wisp3rwind force-pushed the pr_drop_old_python branch 2 times, most recently from 6521789 to d24cf69 Compare December 24, 2022 12:23

wisp3rwind force-pushed the pr_drop_old_python branch 3 times, most recently from c2caa8e to 0a22e28 Compare December 24, 2022 15:13

wisp3rwind force-pushed the pr_drop_old_python branch from a43816f to 82502d0 Compare February 12, 2023 15:04

wisp3rwind added 4 commits February 12, 2023 16:16

library/PathQuery: remove useless bytestring_path()

9052854

normpath already applies bytestring_path() to its output

library/PathQuery: fix lower-casing

427bfe8

drop old Python: simplify and improve case_sensitive()

a666057

- some cleanup for recent Python which has samefile() on Windows - also, fix this function by only upper-/lower-casing one path component at a time. It is unclear to me how this could have ever worked.

wisp3rwind force-pushed the pr_drop_old_python branch from 82502d0 to a666057 Compare February 12, 2023 15:16

sampsyo approved these changes Feb 19, 2023

View reviewed changes

wisp3rwind merged commit 22ca6ef into beetbox:master Feb 19, 2023

wisp3rwind deleted the pr_drop_old_python branch February 19, 2023 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Drop Python 3.6, some Python 2 + Python <= 3.6 cleanup - #4374

Drop Python 3.6, some Python 2 + Python <= 3.6 cleanup#4374
wisp3rwind merged 17 commits into
beetbox:masterfrom
wisp3rwind:pr_drop_old_python

wisp3rwind commented Jun 14, 2022

Uh oh!

sampsyo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wisp3rwind commented Feb 12, 2023

Uh oh!

sampsyo left a comment

Uh oh!

sampsyo Feb 19, 2023

Uh oh!

wisp3rwind Feb 19, 2023

Uh oh!

sampsyo Feb 19, 2023

Uh oh!

sampsyo Feb 19, 2023

Uh oh!

JOJ0 commented Feb 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

wisp3rwind commented Jun 14, 2022

Description

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wisp3rwind commented Feb 12, 2023

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

sampsyo Feb 19, 2023

Choose a reason for hiding this comment

Uh oh!

wisp3rwind Feb 19, 2023

Choose a reason for hiding this comment

Uh oh!

sampsyo Feb 19, 2023

Choose a reason for hiding this comment

Uh oh!

sampsyo Feb 19, 2023

Choose a reason for hiding this comment

Uh oh!

JOJ0 commented Feb 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants