Drop Python 3.6, some Python 2 + Python <= 3.6 cleanup #4374
Conversation
a4bd51b to
acc2cf4
Compare
There was a problem hiding this comment.
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.
04760e4 to
1d06715
Compare
c9c4329 to
f7871c6
Compare
20f842c to
17f8479
Compare
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).
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)
6521789 to
d24cf69
Compare
- 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...
c2caa8e to
0a22e28
Compare
a43816f to
82502d0
Compare
- 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.
82502d0 to
a666057
Compare
|
Ok, this should finally be ready to go: The (This PR is still best studied commit-by-commit, which are well-organized, whereas the scope of the entire PR has grown a lot...) |
There was a problem hiding this comment.
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?
| if cls.force_implicit_query_detection: | ||
| return True |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, I don't really like this flag either, but at least it's obvious what's going on.
| finally: | ||
| self.patcher_exists.start() | ||
| os.chdir(cur_dir) |
There was a problem hiding this comment.
This is some very tricky stuff to get right! Very nice work here. It all looks right to me, though.
| # lowercased. | ||
| # This also ensures that the `match()` method below and the SQL | ||
| # from `col_clause()` do the same thing. | ||
| path = path.lower() |
There was a problem hiding this comment.
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.
|
❤️ |
Description
Fixes #4372; best reviewed commit-by-commit. Don't merge this just yet, I want to review the
case_sensitivechange again (please do so, too!).