From f419543f07cb73d4217cabd07eaa39b6a3414299 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 14 Jun 2022 22:13:54 +0200 Subject: [PATCH 01/17] docs: remove unused link target, update links to python docs --- CONTRIBUTING.rst | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 18ca9b9e4..daf377d94 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -347,7 +347,6 @@ others. See `unittest.mock`_ for more info. ``mock.patch``, as they require manual cleanup. Use the annotation or context manager forms instead. -.. _Python unittest: https://docs.python.org/2/library/unittest.html .. _Codecov: https://codecov.io/github/beetbox/beets .. _pytest-random: https://github.com/klrmn/pytest-random .. _tox: https://tox.readthedocs.io/en/latest/ @@ -358,10 +357,9 @@ others. See `unittest.mock`_ for more info. .. _`https://github.com/beetbox/beets/blob/master/setup.py#L99`: https://github.com/beetbox/beets/blob/master/setup.py#L99 .. _test: https://github.com/beetbox/beets/tree/master/test .. _`https://github.com/beetbox/beets/blob/master/test/test_template.py#L224`: https://github.com/beetbox/beets/blob/master/test/test_template.py#L224 -.. _unittest: https://docs.python.org/3.8/library/unittest.html +.. _unittest: https://docs.python.org/3/library/unittest.html .. _integration test: https://github.com/beetbox/beets/actions?query=workflow%3A%22integration+tests%22 .. _unittest.mock: https://docs.python.org/3/library/unittest.mock.html -.. _Python unittest: https://docs.python.org/2/library/unittest.html .. _documentation: https://beets.readthedocs.io/en/stable/ .. _pip: https://pip.pypa.io/en/stable/ .. _vim: https://www.vim.org/ From bd09cc90b601c3bab01215c85165dbb219bb0467 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 14 Jun 2022 22:15:53 +0200 Subject: [PATCH 02/17] drop Python 3.6: docs, a few safe simplifications --- beets/autotag/hooks.py | 8 +------- beets/util/__init__.py | 8 ++------ beets/util/functemplate.py | 13 +------------ beetsplug/smartplaylist.py | 7 +------ docs/changelog.rst | 4 ++++ docs/guides/main.rst | 6 +++--- setup.py | 2 +- 7 files changed, 13 insertions(+), 35 deletions(-) diff --git a/beets/autotag/hooks.py b/beets/autotag/hooks.py index 30904ff29..8bd87d84a 100644 --- a/beets/autotag/hooks.py +++ b/beets/autotag/hooks.py @@ -28,12 +28,6 @@ from unidecode import unidecode log = logging.getLogger('beets') -# The name of the type for patterns in re changed in Python 3.7. -try: - Pattern = re._pattern_type -except AttributeError: - Pattern = re.Pattern - # Classes used to represent candidate options. class AttrDict(dict): @@ -449,7 +443,7 @@ class Distance: be a compiled regular expression, in which case it will be matched against `value2`. """ - if isinstance(value1, Pattern): + if isinstance(value1, re.Pattern): return bool(value1.match(value2)) return value1 == value2 diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 06e02ee08..fa3b17537 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -868,10 +868,7 @@ def command_output(cmd, shell=False): """ cmd = convert_command_args(cmd) - try: # python >= 3.3 - devnull = subprocess.DEVNULL - except AttributeError: - devnull = open(os.devnull, 'r+b') + devnull = subprocess.DEVNULL proc = subprocess.Popen( cmd, @@ -1054,8 +1051,7 @@ def asciify_path(path, sep_replace): def par_map(transform, items): """Apply the function `transform` to all the elements in the iterable `items`, like `map(transform, items)` but with no return - value. The map *might* happen in parallel: it's parallel on Python 3 - and sequential on Python 2. + value. The parallelism uses threads (not processes), so this is only useful for IO-bound `transform`s. diff --git a/beets/util/functemplate.py b/beets/util/functemplate.py index 289a436de..809207b9a 100644 --- a/beets/util/functemplate.py +++ b/beets/util/functemplate.py @@ -530,18 +530,7 @@ def _parse(template): return Expression(parts) -def cached(func): - """Like the `functools.lru_cache` decorator, but works (as a no-op) - on Python < 3.2. - """ - if hasattr(functools, 'lru_cache'): - return functools.lru_cache(maxsize=128)(func) - else: - # Do nothing when lru_cache is not available. - return func - - -@cached +@functools.lru_cache(maxsize=128) def template(fmt): return Template(fmt) diff --git a/beetsplug/smartplaylist.py b/beetsplug/smartplaylist.py index 4c921eccc..22fce6a63 100644 --- a/beetsplug/smartplaylist.py +++ b/beetsplug/smartplaylist.py @@ -24,12 +24,7 @@ from beets.library import Item, Album, parse_query_string from beets.dbcore import OrQuery from beets.dbcore.query import MultipleSort, ParsingError import os - -try: - from urllib.request import pathname2url -except ImportError: - # python2 is a bit different - from urllib import pathname2url +from urllib.request import pathname2url class SmartPlaylistPlugin(BeetsPlugin): diff --git a/docs/changelog.rst b/docs/changelog.rst index c7f3eb614..f3adcddb4 100755 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -6,6 +6,9 @@ Changelog Changelog goes here! +With this release, beets now requires Python 3.7 or later (it removes support +for Python 3.6). + New features: * We now import the remixer field from Musicbrainz into the library. @@ -125,6 +128,7 @@ Bug fixes: For packagers: +* As noted above, the minimum Python version is now 3.7. * We fixed a version for the dependency on the `Confuse`_ library. :bug:`4167` * The minimum required version of :pypi:`mediafile` is now 0.9.0. diff --git a/docs/guides/main.rst b/docs/guides/main.rst index 2b573ac32..6169ded8c 100644 --- a/docs/guides/main.rst +++ b/docs/guides/main.rst @@ -10,7 +10,7 @@ Installing ---------- You will need Python. -Beets works on Python 3.6 or later. +Beets works on Python 3.7 or later. * **macOS** 11 (Big Sur) includes Python 3.8 out of the box. You can opt for a more recent Python installing it via `Homebrew`_ @@ -94,7 +94,7 @@ Installing on Windows Installing beets on Windows can be tricky. Following these steps might help you get it right: -1. If you don't have it, `install Python`_ (you want at least Python 3.6). The +1. If you don't have it, `install Python`_ (you want at least Python 3.7). The installer should give you the option to "add Python to PATH." Check this box. If you do that, you can skip the next step. @@ -105,7 +105,7 @@ get it right: should open the "System Properties" screen, then select the "Advanced" tab, then hit the "Environmental Variables..." button, and then look for the PATH variable in the table. Add the following to the end of the variable's value: - ``;C:\Python36;C:\Python36\Scripts``. You may need to adjust these paths to + ``;C:\Python37;C:\Python37\Scripts``. You may need to adjust these paths to point to your Python installation. 3. Now install beets by running: ``pip install beets`` diff --git a/setup.py b/setup.py index d49ed65b2..0e8162e4d 100755 --- a/setup.py +++ b/setup.py @@ -166,10 +166,10 @@ setup( 'Environment :: Web Environment', 'Programming Language :: Python', 'Programming Language :: Python :: 3', - 'Programming Language :: Python :: 3.6', 'Programming Language :: Python :: 3.7', 'Programming Language :: Python :: 3.8', 'Programming Language :: Python :: 3.9', + 'Programming Language :: Python :: 3.10', 'Programming Language :: Python :: Implementation :: CPython', ], ) From 632698680f5cc822f616f99e92d058aa6633d2de Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 14 Jun 2022 22:16:43 +0200 Subject: [PATCH 03/17] drop old Python: simplify datetime usage --- beets/dbcore/query.py | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/beets/dbcore/query.py b/beets/dbcore/query.py index b0c769790..a0d79da70 100644 --- a/beets/dbcore/query.py +++ b/beets/dbcore/query.py @@ -526,19 +526,6 @@ class FalseQuery(Query): # Time/date queries. -def _to_epoch_time(date): - """Convert a `datetime` object to an integer number of seconds since - the (local) Unix epoch. - """ - if hasattr(date, 'timestamp'): - # The `timestamp` method exists on Python 3.3+. - return int(date.timestamp()) - else: - epoch = datetime.fromtimestamp(0) - delta = date - epoch - return int(delta.total_seconds()) - - def _parse_periods(pattern): """Parse a string containing two dates separated by two dots (..). Return a pair of `Period` objects. @@ -724,13 +711,15 @@ class DateQuery(FieldQuery): clause_parts = [] subvals = [] + # Convert the `datetime` objects to an integer number of seconds since + # the (local) Unix epoch using `datetime.timestamp()`. if self.interval.start: clause_parts.append(self._clause_tmpl.format(self.field, ">=")) - subvals.append(_to_epoch_time(self.interval.start)) + subvals.append(int(self.interval.start.timestamp())) if self.interval.end: clause_parts.append(self._clause_tmpl.format(self.field, "<")) - subvals.append(_to_epoch_time(self.interval.end)) + subvals.append(int(self.interval.end.timestamp())) if clause_parts: # One- or two-sided interval. From 5cdb0c5c5cc9eb23be1d1a7a540b808b0a11ad16 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 14 Jun 2022 22:17:41 +0200 Subject: [PATCH 04/17] drop old Python: rm python 2 leftovers in library.FileOperationError.__str__ --- beets/library.py | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/beets/library.py b/beets/library.py index 981563974..030cf630e 100644 --- a/beets/library.py +++ b/beets/library.py @@ -300,34 +300,26 @@ class FileOperationError(Exception): self.path = path self.reason = reason - def text(self): + def __str__(self): """Get a string representing the error. - Describe both the underlying reason and the file path - in question. + Describe both the underlying reason and the file path in question. """ - return '{}: {}'.format( - util.displayable_path(self.path), - str(self.reason) - ) - - # define __str__ as text to avoid infinite loop on super() calls - # with @six.python_2_unicode_compatible - __str__ = text + return f"{util.displayable_path(self.path)}: {self.reason}" class ReadError(FileOperationError): """An error while reading a file (i.e. in `Item.read`).""" def __str__(self): - return 'error reading ' + super().text() + return 'error reading ' + str(super()) class WriteError(FileOperationError): """An error while writing a file (i.e. in `Item.write`).""" def __str__(self): - return 'error writing ' + super().text() + return 'error writing ' + str(super()) # Item and Album model classes. From 67f0c73eecbe5b2645e6c7fb8e6ad52e140d2db0 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 14 Jun 2022 22:20:38 +0200 Subject: [PATCH 05/17] drop old Python: don't handle obsolte exception in keyfinder plugin --- beetsplug/keyfinder.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/beetsplug/keyfinder.py b/beetsplug/keyfinder.py index b695ab54e..051af9e1f 100644 --- a/beetsplug/keyfinder.py +++ b/beetsplug/keyfinder.py @@ -67,12 +67,6 @@ class KeyFinderPlugin(BeetsPlugin): except (subprocess.CalledProcessError, OSError) as exc: self._log.error('execution failed: {0}', exc) continue - except UnicodeEncodeError: - # Workaround for Python 2 Windows bug. - # https://bugs.python.org/issue1759845 - self._log.error('execution failed for Unicode path: {0!r}', - item.path) - continue try: key_raw = output.rsplit(None, 1)[-1] From 22826c6d36ef73f9d688a154fe8c3f3993c47ffb Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 14 Jun 2022 22:21:14 +0200 Subject: [PATCH 06/17] drop old Python: remove obsolete custom string formatter in hooks plugin --- beetsplug/hook.py | 38 +++++++++----------------------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/beetsplug/hook.py b/beetsplug/hook.py index 0fe3bffc6..8c7211450 100644 --- a/beetsplug/hook.py +++ b/beetsplug/hook.py @@ -26,39 +26,20 @@ class CodingFormatter(string.Formatter): """A variant of `string.Formatter` that converts everything to `unicode` strings. - This is necessary on Python 2, where formatting otherwise occurs on - bytestrings. It intercepts two points in the formatting process to decode - the format string and all fields using the specified encoding. If decoding - fails, the values are used as-is. + This was necessary on Python 2, in needs to be kept for backwards + compatibility. """ def __init__(self, coding): """Creates a new coding formatter with the provided coding.""" self._coding = coding - def format(self, format_string, *args, **kwargs): - """Formats the provided string using the provided arguments and keyword - arguments. - - This method decodes the format string using the formatter's coding. - - See str.format and string.Formatter.format. - """ - if isinstance(format_string, bytes): - format_string = format_string.decode(self._coding) - - return super().format(format_string, *args, - **kwargs) - def convert_field(self, value, conversion): """Converts the provided value given a conversion type. This method decodes the converted value using the formatter's coding. - - See string.Formatter.convert_field. """ - converted = super().convert_field(value, - conversion) + converted = super().convert_field(value, conversion) if isinstance(converted, bytes): return converted.decode(self._coding) @@ -92,14 +73,13 @@ class HookPlugin(BeetsPlugin): self._log.error('invalid command "{0}"', command) return - # Use a string formatter that works on Unicode strings. + # For backwards compatibility, use a string formatter that decodes + # bytes (in particular, paths) to unicode strings. formatter = CodingFormatter(arg_encoding()) - - command_pieces = shlex.split(command) - - for i, piece in enumerate(command_pieces): - command_pieces[i] = formatter.format(piece, event=event, - **kwargs) + command_pieces = [ + formatter.format(piece, event=event, **kwargs) + for piece in shlex.split(command) + ] self._log.debug('running command "{0}" for event {1}', ' '.join(command_pieces), event) From c816b2953d176a728d1ac91b1ca7cd86cb9291ce Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 14 Jun 2022 22:21:58 +0200 Subject: [PATCH 07/17] drop old Python: remove obsolete exception handling for Windows symlinks when symlinks aren't supported, recent python will only raise NotImplementedError (or might even support symlinks), ending up in the other except arm --- beets/util/__init__.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index fa3b17537..1f2b57607 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -532,10 +532,6 @@ def link(path, dest, replace=False): raise FilesystemError('OS does not support symbolic links.' 'link', (path, dest), traceback.format_exc()) except OSError as exc: - # TODO: Windows version checks can be removed for python 3 - if hasattr('sys', 'getwindowsversion'): - if sys.getwindowsversion()[0] < 6: # is before Vista - exc = 'OS does not support symbolic links.' raise FilesystemError(exc, 'link', (path, dest), traceback.format_exc()) From 9f5867343342d1178cdad0440882a7348e2a6d7f Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 14 Jun 2022 22:24:49 +0200 Subject: [PATCH 08/17] drop old Python: update a bunch of comments, removing Python 2 details These didn't match the code anymore (which had already been changes manually or automatically as part of the 2->3 transition). --- beets/logging.py | 3 +++ beets/util/__init__.py | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/beets/logging.py b/beets/logging.py index 516528c05..9e88fd7c1 100644 --- a/beets/logging.py +++ b/beets/logging.py @@ -21,6 +21,9 @@ that when getLogger(name) instantiates a logger that logger uses """ +# FIXME: Remove Python 2 leftovers. + + from copy import copy import subprocess import threading diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 1f2b57607..07ef021ea 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -737,8 +737,7 @@ def legalize_path(path, replacements, length, extension, fragment): def py3_path(path): - """Convert a bytestring path to Unicode on Python 3 only. On Python - 2, return the bytestring path unchanged. + """Convert a bytestring path to Unicode. This helps deal with APIs on Python 3 that *only* accept Unicode (i.e., `str` objects). I philosophically disagree with this From 7c2aa02de2dfc26765e27825c0ee48634d070ec6 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Wed, 15 Jun 2022 11:09:21 +0200 Subject: [PATCH 09/17] remove old Python: in logging, rm workarounds for broken string conversions --- beets/logging.py | 36 ++++++++---------------------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/beets/logging.py b/beets/logging.py index 9e88fd7c1..63ecf1c3f 100644 --- a/beets/logging.py +++ b/beets/logging.py @@ -25,48 +25,28 @@ that when getLogger(name) instantiates a logger that logger uses from copy import copy -import subprocess import threading import logging def logsafe(val): - """Coerce a potentially "problematic" value so it can be formatted - in a Unicode log string. + """Coerce `bytes` to `str` to avoid crashes solely due to logging. - This works around a number of pitfalls when logging objects in - Python 2: - - Logging path names, which must be byte strings, requires - conversion for output. - - Some objects, including some exceptions, will crash when you call - `unicode(v)` while `str(v)` works fine. CalledProcessError is an - example. + This is particularly relevant for bytestring paths. Much of our code + explicitly uses `displayable_path` for them, but better be safe and prevent + any crashes that are solely due to log formatting. """ - # Already Unicode. - if isinstance(val, str): - return val - - # Bytestring: needs decoding. - elif isinstance(val, bytes): + # Bytestring: Needs decoding to be safe for substitution in format strings. + if isinstance(val, bytes): # Blindly convert with UTF-8. Eventually, it would be nice to # (a) only do this for paths, if they can be given a distinct # type, and (b) warn the developer if they do this for other # bytestrings. return val.decode('utf-8', 'replace') - # A "problem" object: needs a workaround. - elif isinstance(val, subprocess.CalledProcessError): - try: - return str(val) - except UnicodeDecodeError: - # An object with a broken __unicode__ formatter. Use __str__ - # instead. - return str(val).decode('utf-8', 'replace') - # Other objects are used as-is so field access, etc., still works in - # the format string. - else: - return val + # the format string. Relies on a working __str__ implementation. + return val class StrFormatLogger(logging.Logger): From 35e167f75ee6acae9d9f0ea217b26855d7e4413f Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Wed, 15 Jun 2022 11:33:38 +0200 Subject: [PATCH 10/17] remove old Python: in logging, update comments and support new kwargs 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. --- beets/logging.py | 45 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/beets/logging.py b/beets/logging.py index 63ecf1c3f..05c22bd1c 100644 --- a/beets/logging.py +++ b/beets/logging.py @@ -12,19 +12,17 @@ # The above copyright notice and this permission notice shall be # included in all copies or substantial portions of the Software. -"""A drop-in replacement for the standard-library `logging` module that -allows {}-style log formatting on Python 2 and 3. +"""A drop-in replacement for the standard-library `logging` module. -Provides everything the "logging" module does. The only difference is -that when getLogger(name) instantiates a logger that logger uses -{}-style formatting. +Provides everything the "logging" module does. In addition, beets' logger +(as obtained by `getLogger(name)`) supports thread-local levels, and messages +use {}-style formatting and can interpolate keywords arguments to the logging +calls (`debug`, `info`, etc). """ -# FIXME: Remove Python 2 leftovers. - - from copy import copy +import sys import threading import logging @@ -51,7 +49,14 @@ def logsafe(val): class StrFormatLogger(logging.Logger): """A version of `Logger` that uses `str.format`-style formatting - instead of %-style formatting. + instead of %-style formatting and supports keyword arguments. + + We cannot easily get rid of this even in the Python 3 era: This custom + formatting supports substitution from `kwargs` into the message, which the + default `logging.Logger._log()` implementation does not. + + Remark by @sampsyo: https://stackoverflow.com/a/24683360 might be a way to + achieve this with less code. """ class _LogMessage: @@ -65,10 +70,28 @@ class StrFormatLogger(logging.Logger): kwargs = {k: logsafe(v) for (k, v) in self.kwargs.items()} return self.msg.format(*args, **kwargs) - def _log(self, level, msg, args, exc_info=None, extra=None, **kwargs): + def _log(self, level, msg, args, exc_info=None, extra=None, + stack_info=False, **kwargs): """Log msg.format(*args, **kwargs)""" m = self._LogMessage(msg, args, kwargs) - return super()._log(level, m, (), exc_info, extra) + + stacklevel = kwargs.pop("stacklevel", 1) + if sys.version_info >= (3, 8): + stacklevel = {"stacklevel": stacklevel} + else: + # Simply ignore this when not supported by current Python version. + # Can be dropped when we remove support for Python 3.7. + stacklevel = {} + + return super()._log( + level, + m, + (), + exc_info=exc_info, + extra=extra, + stack_info=stack_info, + **stacklevel, + ) class ThreadLocalLevelLogger(logging.Logger): From 3510e6311db479471102b5f0380d27fe2be8786b Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Sat, 24 Sep 2022 12:08:04 +0200 Subject: [PATCH 11/17] web: slight refactor to make path handling more readable (at least, more readable to me) --- beetsplug/web/__init__.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/beetsplug/web/__init__.py b/beetsplug/web/__init__.py index b7baa93c1..6a70a962d 100644 --- a/beetsplug/web/__init__.py +++ b/beetsplug/web/__init__.py @@ -307,19 +307,24 @@ def item_file(item_id): else: item_path = util.py3_path(item.path) - try: - unicode_item_path = util.text_string(item.path) - except (UnicodeDecodeError, UnicodeEncodeError): - unicode_item_path = util.displayable_path(item.path) + base_filename = os.path.basename(item_path) + # FIXME: Arguably, this should just use `displayable_path`: The latter + # tries `_fsencoding()` first, but then falls back to `utf-8`, too. + if isinstance(base_filename, bytes): + try: + unicode_base_filename = base_filename.decode("utf-8") + except UnicodeError: + unicode_base_filename = util.displayable_path(base_filename) + else: + unicode_base_filename = base_filename - base_filename = os.path.basename(unicode_item_path) try: # Imitate http.server behaviour - base_filename.encode("latin-1", "strict") - except UnicodeEncodeError: - safe_filename = unidecode(base_filename) + unicode_base_filename.encode("latin-1", "strict") + except UnicodeError: + safe_filename = unidecode(unicode_base_filename) else: - safe_filename = base_filename + safe_filename = unicode_base_filename response = flask.send_file( item_path, From d24cf692693045114fd822eebd1841b98dc90143 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Wed, 15 Jun 2022 12:15:24 +0200 Subject: [PATCH 12/17] remove old Python: remove util.text_string 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) --- beets/ui/__init__.py | 3 --- beets/ui/commands.py | 4 ++-- beets/util/__init__.py | 13 ------------- beetsplug/keyfinder.py | 2 +- beetsplug/web/__init__.py | 4 ++-- test/helper.py | 2 +- test/test_ui.py | 3 +-- 7 files changed, 7 insertions(+), 24 deletions(-) diff --git a/beets/ui/__init__.py b/beets/ui/__init__.py index ba058148d..cd9f4989e 100644 --- a/beets/ui/__init__.py +++ b/beets/ui/__init__.py @@ -790,9 +790,6 @@ def _store_dict(option, opt_str, value, parser): setattr(parser.values, dest, {}) option_values = getattr(parser.values, dest) - # Decode the argument using the platform's argument encoding. - value = util.text_string(value, util.arg_encoding()) - try: key, value = value.split('=', 1) if not (key and value): diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 91cee4516..6c8e25b85 100755 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -1778,7 +1778,7 @@ def config_func(lib, opts, args): else: config_out = config.dump(full=opts.defaults, redact=opts.redact) if config_out.strip() != '{}': - print_(util.text_string(config_out)) + print_(config_out) else: print("Empty configuration") @@ -1852,7 +1852,7 @@ def completion_script(commands): """ base_script = os.path.join(os.path.dirname(__file__), 'completion_base.sh') with open(base_script) as base_script: - yield util.text_string(base_script.read()) + yield base_script.read() options = {} aliases = {} diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 07ef021ea..fbff07660 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -770,19 +770,6 @@ def as_string(value): return str(value) -def text_string(value, encoding='utf-8'): - """Convert a string, which can either be bytes or unicode, to - unicode. - - Text (unicode) is left untouched; bytes are decoded. This is useful - to convert from a "native string" (bytes on Python 2, str on Python - 3) to a consistently unicode value. - """ - if isinstance(value, bytes): - return value.decode(encoding) - return value - - def plurality(objs): """Given a sequence of hashble objects, returns the object that is most common in the set and the its number of appearance. The diff --git a/beetsplug/keyfinder.py b/beetsplug/keyfinder.py index 051af9e1f..412b7ddaa 100644 --- a/beetsplug/keyfinder.py +++ b/beetsplug/keyfinder.py @@ -77,7 +77,7 @@ class KeyFinderPlugin(BeetsPlugin): continue try: - key = util.text_string(key_raw) + key = key_raw.decode("utf-8") except UnicodeDecodeError: self._log.error('output is invalid UTF-8') continue diff --git a/beetsplug/web/__init__.py b/beetsplug/web/__init__.py index 6a70a962d..7b04f3714 100644 --- a/beetsplug/web/__init__.py +++ b/beetsplug/web/__init__.py @@ -320,9 +320,9 @@ def item_file(item_id): try: # Imitate http.server behaviour - unicode_base_filename.encode("latin-1", "strict") + base_filename.encode("latin-1", "strict") except UnicodeError: - safe_filename = unidecode(unicode_base_filename) + safe_filename = unidecode(base_filename) else: safe_filename = unicode_base_filename diff --git a/test/helper.py b/test/helper.py index f7d37b654..b6e425c62 100644 --- a/test/helper.py +++ b/test/helper.py @@ -453,7 +453,7 @@ class TestHelper: def run_with_output(self, *args): with capture_stdout() as out: self.run_command(*args) - return util.text_string(out.getvalue()) + return out.getvalue() # Safe file operations diff --git a/test/test_ui.py b/test/test_ui.py index ad4387013..2034fb41f 100644 --- a/test/test_ui.py +++ b/test/test_ui.py @@ -1163,8 +1163,7 @@ class ShowChangeTest(_common.TestCase): cur_album, autotag.AlbumMatch(album_dist, info, mapping, set(), set()), ) - # FIXME decoding shouldn't be done here - return util.text_string(self.io.getoutput().lower()) + return self.io.getoutput().lower() def test_null_change(self): msg = self._show_change() From a6d74686d8435ee3215c8c11af856b4cb7b814c1 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Sat, 24 Dec 2022 13:36:53 +0100 Subject: [PATCH 13/17] test: separate case_sensitive unit tests from PathQueryTest - 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... --- test/test_query.py | 33 --------------------------------- test/test_util.py | 27 +++++++++++++++++++++++++-- 2 files changed, 25 insertions(+), 35 deletions(-) diff --git a/test/test_query.py b/test/test_query.py index 3c6d6f70a..13f40482b 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -603,40 +603,7 @@ class PathQueryTest(_common.LibTestCase, TestHelper, AssertsMixin): results = self.lib.items(makeq(case_sensitive=False)) self.assert_items_matched(results, ['path item', 'caps path']) - # Check for correct case sensitivity selection (this check - # only works on non-Windows OSes). - with _common.system_mock('Darwin'): - # exists = True and samefile = True => Case insensitive - q = makeq() - self.assertEqual(q.case_sensitive, False) - # exists = True and samefile = False => Case sensitive - self.patcher_samefile.stop() - self.patcher_samefile.start().return_value = False - try: - q = makeq() - self.assertEqual(q.case_sensitive, True) - finally: - self.patcher_samefile.stop() - self.patcher_samefile.start().return_value = True - - # Test platform-aware default sensitivity when the library path - # does not exist. For the duration of this check, we change the - # `os.path.exists` mock to return False. - self.patcher_exists.stop() - self.patcher_exists.start().return_value = False - try: - with _common.system_mock('Darwin'): - q = makeq() - self.assertEqual(q.case_sensitive, True) - - with _common.system_mock('Windows'): - q = makeq() - self.assertEqual(q.case_sensitive, False) - finally: - # Restore the `os.path.exists` mock to its original state. - self.patcher_exists.stop() - self.patcher_exists.start().return_value = True @patch('beets.library.os') def test_path_sep_detection(self, mock_os): diff --git a/test/test_util.py b/test/test_util.py index 14ac7f2b2..8c16243a5 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -14,10 +14,11 @@ """Tests for base utils from the beets.util package. """ -import sys -import re import os +import platform +import re import subprocess +import sys import unittest from unittest.mock import patch, Mock @@ -122,6 +123,28 @@ class UtilTest(unittest.TestCase): self.assertEqual(exc_context.exception.returncode, 1) self.assertEqual(exc_context.exception.cmd, 'taga \xc3\xa9') + def test_case_sensitive_default(self): + path = util.bytestring_path(util.normpath( + "/this/path/does/not/exist", + )) + + self.assertEqual( + util.case_sensitive(path), + platform.system() != 'Windows', + ) + + @unittest.skipIf(sys.platform == 'win32', 'fs is not case sensitive') + def test_case_sensitive_detects_sensitive(self): + # FIXME: Add tests for more code paths of case_sensitive() + # when the filesystem on the test runner is not case sensitive + pass + + @unittest.skipIf(sys.platform != 'win32', 'fs is case sensitive') + def test_case_sensitive_detects_insensitive(self): + # FIXME: Add tests for more code paths of case_sensitive() + # when the filesystem on the test runner is case sensitive + pass + class PathConversionTest(_common.TestCase): def test_syspath_windows_format(self): From e6fd038b0edd760b84163f696324ccc3069c5b8c Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Sat, 24 Dec 2022 14:19:41 +0100 Subject: [PATCH 14/17] tests: robustify path query / case_sensitive tests - 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 --- beets/library.py | 14 +++-- test/test_query.py | 152 ++++++++++++++++++++++----------------------- 2 files changed, 82 insertions(+), 84 deletions(-) diff --git a/beets/library.py b/beets/library.py index 030cf630e..b9344fe89 100644 --- a/beets/library.py +++ b/beets/library.py @@ -51,6 +51,8 @@ class PathQuery(dbcore.FieldQuery): default, the behavior depends on the OS: case-insensitive on Windows and case-sensitive otherwise. """ + # For tests + force_implicit_query_detection = False def __init__(self, field, pattern, fast=True, case_sensitive=None): """Create a path query. @@ -90,11 +92,13 @@ class PathQuery(dbcore.FieldQuery): # Test both `sep` and `altsep` (i.e., both slash and backslash on # Windows). - return ( - (os.sep in query_part or - (os.altsep and os.altsep in query_part)) and - os.path.exists(syspath(normpath(query_part))) - ) + if not (os.sep in query_part + or (os.altsep and os.altsep in query_part)): + return False + + if cls.force_implicit_query_detection: + return True + return os.path.exists(syspath(normpath(query_part))) def match(self, item): path = item.path if self.case_sensitive else item.path.lower() diff --git a/test/test_query.py b/test/test_query.py index 13f40482b..3d7b56781 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -15,8 +15,8 @@ """Various tests for querying the library database. """ +from contextlib import contextmanager from functools import partial -from unittest.mock import patch import os import sys import unittest @@ -454,23 +454,14 @@ class PathQueryTest(_common.LibTestCase, TestHelper, AssertsMixin): self.lib.add(i2) self.lib.add_album([i2]) + @contextmanager + def force_implicit_query_detection(self): # Unadorned path queries with path separators in them are considered # path queries only when the path in question actually exists. So we # mock the existence check to return true. - self.patcher_exists = patch('beets.library.os.path.exists') - self.patcher_exists.start().return_value = True - - # We have to create function samefile as it does not exist on - # Windows and python 2.7 - self.patcher_samefile = patch('beets.library.os.path.samefile', - create=True) - self.patcher_samefile.start().return_value = True - - def tearDown(self): - super().tearDown() - - self.patcher_samefile.stop() - self.patcher_exists.stop() + beets.library.PathQuery.force_implicit_query_detection = True + yield + beets.library.PathQuery.force_implicit_query_detection = False def test_path_exact_match(self): q = 'path:/a/b/c.mp3' @@ -526,31 +517,35 @@ class PathQueryTest(_common.LibTestCase, TestHelper, AssertsMixin): @unittest.skipIf(sys.platform == 'win32', WIN32_NO_IMPLICIT_PATHS) def test_slashed_query_matches_path(self): - q = '/a/b' - results = self.lib.items(q) - self.assert_items_matched(results, ['path item']) + with self.force_implicit_query_detection(): + q = '/a/b' + results = self.lib.items(q) + self.assert_items_matched(results, ['path item']) - results = self.lib.albums(q) - self.assert_albums_matched(results, ['path album']) + results = self.lib.albums(q) + self.assert_albums_matched(results, ['path album']) @unittest.skipIf(sys.platform == 'win32', WIN32_NO_IMPLICIT_PATHS) def test_path_query_in_or_query(self): - q = '/a/b , /a/b' - results = self.lib.items(q) - self.assert_items_matched(results, ['path item']) + with self.force_implicit_query_detection(): + q = '/a/b , /a/b' + results = self.lib.items(q) + self.assert_items_matched(results, ['path item']) def test_non_slashed_does_not_match_path(self): - q = 'c.mp3' - results = self.lib.items(q) - self.assert_items_matched(results, []) + with self.force_implicit_query_detection(): + q = 'c.mp3' + results = self.lib.items(q) + self.assert_items_matched(results, []) - results = self.lib.albums(q) - self.assert_albums_matched(results, []) + results = self.lib.albums(q) + self.assert_albums_matched(results, []) def test_slashes_in_explicit_field_does_not_match_path(self): - q = 'title:/a/b' - results = self.lib.items(q) - self.assert_items_matched(results, []) + with self.force_implicit_query_detection(): + q = 'title:/a/b' + results = self.lib.items(q) + self.assert_items_matched(results, []) def test_path_item_regex(self): q = 'path::c\\.mp3$' @@ -603,68 +598,67 @@ class PathQueryTest(_common.LibTestCase, TestHelper, AssertsMixin): results = self.lib.items(makeq(case_sensitive=False)) self.assert_items_matched(results, ['path item', 'caps path']) + # FIXME: Also create a variant of this test for windows, which tests + # both os.sep and os.altsep + @unittest.skipIf(sys.platform == 'win32', 'win32') + def test_path_sep_detection(self): + is_path_query = beets.library.PathQuery.is_path_query + with self.force_implicit_query_detection(): + self.assertTrue(is_path_query('/foo/bar')) + self.assertTrue(is_path_query('foo/bar')) + self.assertTrue(is_path_query('foo/')) + self.assertFalse(is_path_query('foo')) + self.assertTrue(is_path_query('foo/:bar')) + self.assertFalse(is_path_query('foo:bar/')) + self.assertFalse(is_path_query('foo:/bar')) - @patch('beets.library.os') - def test_path_sep_detection(self, mock_os): - mock_os.sep = '/' - mock_os.altsep = None - mock_os.path.exists = lambda p: True - is_path = beets.library.PathQuery.is_path_query - - self.assertTrue(is_path('/foo/bar')) - self.assertTrue(is_path('foo/bar')) - self.assertTrue(is_path('foo/')) - self.assertFalse(is_path('foo')) - self.assertTrue(is_path('foo/:bar')) - self.assertFalse(is_path('foo:bar/')) - self.assertFalse(is_path('foo:/bar')) - + # FIXME: shouldn't this also work on windows? @unittest.skipIf(sys.platform == 'win32', WIN32_NO_IMPLICIT_PATHS) def test_detect_absolute_path(self): - # Don't patch `os.path.exists`; we'll actually create a file when - # it exists. - self.patcher_exists.stop() - is_path = beets.library.PathQuery.is_path_query + """Test detection of implicit path queries based on whether or + not the path actually exists, when using an absolute path query. - try: - path = self.touch(os.path.join(b'foo', b'bar')) - path = path.decode('utf-8') + Thus, don't use the `force_implicit_query_detection()` + contextmanager which would disable the existence check. + """ + is_path_query = beets.library.PathQuery.is_path_query - # The file itself. - self.assertTrue(is_path(path)) + path = self.touch(os.path.join(b'foo', b'bar')) + self.assertTrue(os.path.isabs(util.syspath(path))) + path_str = path.decode('utf-8') - # The parent directory. - parent = os.path.dirname(path) - self.assertTrue(is_path(parent)) + # The file itself. + self.assertTrue(is_path_query(path_str)) - # Some non-existent path. - self.assertFalse(is_path(path + 'baz')) + # The parent directory. + parent = os.path.dirname(path_str) + self.assertTrue(is_path_query(parent)) - finally: - # Restart the `os.path.exists` patch. - self.patcher_exists.start() + # Some non-existent path. + self.assertFalse(is_path_query(path_str + 'baz')) def test_detect_relative_path(self): - self.patcher_exists.stop() - is_path = beets.library.PathQuery.is_path_query + """Test detection of implicit path queries based on whether or + not the path actually exists, when using a relative path query. + Thus, don't use the `force_implicit_query_detection()` + contextmanager which would disable the existence check. + """ + is_path_query = beets.library.PathQuery.is_path_query + + self.touch(os.path.join(b'foo', b'bar')) + + # Temporarily change directory so relative paths work. + cur_dir = os.getcwd() try: - self.touch(os.path.join(b'foo', b'bar')) - - # Temporarily change directory so relative paths work. - cur_dir = os.getcwd() - try: - os.chdir(self.temp_dir) - self.assertTrue(is_path('foo/')) - self.assertTrue(is_path('foo/bar')) - self.assertTrue(is_path('foo/bar:tagada')) - self.assertFalse(is_path('bar')) - finally: - os.chdir(cur_dir) - + os.chdir(self.temp_dir) + self.assertTrue(is_path_query('foo/')) + self.assertTrue(is_path_query('foo/bar')) + self.assertTrue(is_path_query('foo/bar:tagada')) + self.assertFalse(is_path_query('bar')) finally: - self.patcher_exists.start() + os.chdir(cur_dir) class IntQueryTest(unittest.TestCase, TestHelper): From 9052854e5024f85e2bfe0ae6d972e2a5a4b39b59 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Sat, 24 Dec 2022 18:19:06 +0100 Subject: [PATCH 15/17] library/PathQuery: remove useless bytestring_path() normpath already applies bytestring_path() to its output --- beets/library.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/beets/library.py b/beets/library.py index b9344fe89..c3b11113c 100644 --- a/beets/library.py +++ b/beets/library.py @@ -67,7 +67,7 @@ class PathQuery(dbcore.FieldQuery): # By default, the case sensitivity depends on the filesystem # that the query path is located on. if case_sensitive is None: - path = util.bytestring_path(util.normpath(pattern)) + path = util.normpath(pattern) case_sensitive = beets.util.case_sensitive(path) self.case_sensitive = case_sensitive @@ -76,9 +76,9 @@ class PathQuery(dbcore.FieldQuery): pattern = pattern.lower() # Match the path as a single file. - self.file_path = util.bytestring_path(util.normpath(pattern)) + self.file_path = util.normpath(pattern) # As a directory (prefix). - self.dir_path = util.bytestring_path(os.path.join(self.file_path, b'')) + self.dir_path = os.path.join(self.file_path, b'') @classmethod def is_path_query(cls, query_part): From 427bfe8cbf0b7d06bbc6051b256898b69acebdb4 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Sat, 24 Dec 2022 18:20:14 +0100 Subject: [PATCH 16/17] library/PathQuery: fix lower-casing --- beets/library.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/beets/library.py b/beets/library.py index c3b11113c..be6d9817a 100644 --- a/beets/library.py +++ b/beets/library.py @@ -64,21 +64,27 @@ class PathQuery(dbcore.FieldQuery): """ super().__init__(field, pattern, fast) + path = util.normpath(pattern) + # By default, the case sensitivity depends on the filesystem # that the query path is located on. if case_sensitive is None: - path = util.normpath(pattern) - case_sensitive = beets.util.case_sensitive(path) + case_sensitive = util.case_sensitive(path) self.case_sensitive = case_sensitive # Use a normalized-case pattern for case-insensitive matches. if not case_sensitive: - pattern = pattern.lower() + # We need to lowercase the entire path, not just the pattern. + # In particular, on Windows, the drive letter is otherwise not + # lowercased. + # This also ensures that the `match()` method below and the SQL + # from `col_clause()` do the same thing. + path = path.lower() # Match the path as a single file. - self.file_path = util.normpath(pattern) + self.file_path = path # As a directory (prefix). - self.dir_path = os.path.join(self.file_path, b'') + self.dir_path = os.path.join(path, b'') @classmethod def is_path_query(cls, query_part): From a666057fdf2fa2b102a5badf4743489b3811d818 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 14 Jun 2022 22:19:07 +0200 Subject: [PATCH 17/17] drop old Python: simplify and improve case_sensitive() - 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. --- beets/util/__init__.py | 81 +++++++++++++++++++----------------------- 1 file changed, 36 insertions(+), 45 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index fbff07660..2319890a3 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -936,61 +936,52 @@ def interactive_open(targets, command): return os.execlp(*args) -def _windows_long_path_name(short_path): - """Use Windows' `GetLongPathNameW` via ctypes to get the canonical, - long path given a short filename. - """ - if not isinstance(short_path, str): - short_path = short_path.decode(_fsencoding()) - - import ctypes - buf = ctypes.create_unicode_buffer(260) - get_long_path_name_w = ctypes.windll.kernel32.GetLongPathNameW - return_value = get_long_path_name_w(short_path, buf, 260) - - if return_value == 0 or return_value > 260: - # An error occurred - return short_path - else: - long_path = buf.value - # GetLongPathNameW does not change the case of the drive - # letter. - if len(long_path) > 1 and long_path[1] == ':': - long_path = long_path[0].upper() + long_path[1:] - return long_path - - def case_sensitive(path): """Check whether the filesystem at the given path is case sensitive. To work best, the path should point to a file or a directory. If the path does not exist, assume a case sensitive file system on every platform except Windows. + + Currently only used for absolute paths by beets; may have a trailing + path separator. """ - # A fallback in case the path does not exist. - if not os.path.exists(syspath(path)): - # By default, the case sensitivity depends on the platform. - return platform.system() != 'Windows' + # Look at parent paths until we find a path that actually exists, or + # reach the root. + while True: + head, tail = os.path.split(path) + if head == path: + # We have reached the root of the file system. + # By default, the case sensitivity depends on the platform. + return platform.system() != 'Windows' - # If an upper-case version of the path exists but a lower-case - # version does not, then the filesystem must be case-sensitive. - # (Otherwise, we have more work to do.) - if not (os.path.exists(syspath(path.lower())) and - os.path.exists(syspath(path.upper()))): - return True + # Trailing path separator, or path does not exist. + if not tail or not os.path.exists(path): + path = head + continue - # Both versions of the path exist on the file system. Check whether - # they refer to different files by their inodes. Alas, - # `os.path.samefile` is only available on Unix systems on Python 2. - if platform.system() != 'Windows': - return not os.path.samefile(syspath(path.lower()), - syspath(path.upper())) + upper_tail = tail.upper() + lower_tail = tail.lower() - # On Windows, we check whether the canonical, long filenames for the - # files are the same. - lower = _windows_long_path_name(path.lower()) - upper = _windows_long_path_name(path.upper()) - return lower != upper + # In case we can't tell from the given path name, look at the + # parent directory. + if upper_tail == lower_tail: + path = head + continue + + upper_sys = syspath(os.path.join(head, upper_tail)) + lower_sys = syspath(os.path.join(head, lower_tail)) + + # If either the upper-cased or lower-cased path does not exist, the + # filesystem must be case-sensitive. + # (Otherwise, we have more work to do.) + if not os.path.exists(upper_sys) or not os.path.exists(lower_sys): + return True + + # Original and both upper- and lower-cased versions of the path + # exist on the file system. Check whether they refer to different + # files by their inodes (or an alternative method on Windows). + return not os.path.samefile(lower_sys, upper_sys) def raw_seconds_short(string):