Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Py3: Convert to pathlib #1814

Merged
merged 13 commits into from Oct 18, 2019
Merged

Py3: Convert to pathlib #1814

merged 13 commits into from Oct 18, 2019

Conversation

@jodal
Copy link
Member

jodal commented Oct 8, 2019

Copying in my own comment from #1809 earlier tonight...


Python 3 has the pathlib module, and PyPI has the pathlib2 backport.

Testing them out, it seems they behave quite well on files containing invalid UTF-8 byte sequences in the file name.

On Python 2:

>>> from pathlib2 import Path
>>> p = Path('.')
>>> list(p.glob('foo*'))
[PosixPath('foo\xc3('), PosixPath('foo\xc3(\xc3')]
>>> f = list(p.glob('foo*'))[1]
>>> bytes(f)
'foo\xc3(\xc3'   # The correct bytes
>>> str(f)
'foo\xc3(\xc3'   # Exactly the same, as bytes == str on Python 2
>>> bytes(f).decode('utf-8', 'replace')
u'foo\ufffd(\ufffd'  # Unicode, ready for display, with invalid bytes replaced
>>> print(bytes(f).decode('utf-8', 'replace'))
foo�(�

On Python 3:

>>> from pathlib import Path
>>> p = Path('.')
>>> list(p.glob('foo*'))
[PosixPath('foo\udcc3('), PosixPath('foo\udcc3(\udcc3')]    # Surrogate escapes in action
>>> f = list(p.glob('foo*'))[1]
>>> bytes(f)
b'foo\xc3(\xc3'    # The correct bytes, just with Python 3's b'' prefix
>>> str(f)
'foo\udcc3(\udcc3'    # Surrogate escapes again. This isn't "printable".
>>> bytes(f).decode('utf-8', 'replace')
'foo�(�'    # Unicode, ready for display, with invalid bytes replaced
>>> print(bytes(f).decode('utf-8', 'replace'))
foo�(�

This PR converts everything covered by tests plus some bugs found during manual testing of the lightly tested Mopidy-File backend to use pathlib instead of our old every-path-is-a-bytestring regime, which was a quite leaky regime.

With these changes, we end up:

  • passing pathlib.Path objects around when we deal with paths,
  • when we display Path objects, we either use repr() or make sure to do the replace-dance (done twice in the Mopidy-File backend),
  • when we convert to file:// or local: URIs, we URI encode properly (path.as_uri(), and you're done).

With this, we should be in a very good place with regards to path handling, both for Python 3 and for Windows.

TODO

  • mopidy.m3u. I'm fairly certain it is broken now, as it has zero test coverage.
  • mopidy.audio.scanner, which might not have any test coverage now that Mopidy-Local has moved out.
  • mopidy.internal.mtimes, which is only used by Mopidy-Local and the scanner tests.
@jodal jodal added this to the v3.0 milestone Oct 8, 2019
@jodal jodal requested review from adamcik and kingosticks Oct 8, 2019
@jodal jodal self-assigned this Oct 8, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #1814 into develop will decrease coverage by 0.05%.
The diff coverage is 86.44%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1814      +/-   ##
===========================================
- Coverage    82.12%   82.06%   -0.06%     
===========================================
  Files           77       78       +1     
  Lines         6396     6358      -38     
===========================================
- Hits          5253     5218      -35     
+ Misses        1143     1140       -3
Impacted Files Coverage Δ
mopidy/__main__.py 0% <0%> (ø) ⬆️
mopidy/internal/path.py 100% <100%> (+2.58%) ⬆️
mopidy/compat.py 95.71% <100%> (+0.12%) ⬆️
mopidy/internal/xdg.py 96.55% <100%> (-0.6%) ⬇️
mopidy/file/library.py 37.07% <18.75%> (-1.39%) ⬇️
mopidy/internal/mtimes.py 96.49% <96.49%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68ba9f0...605d462. Read the comment docs.

@jodal jodal force-pushed the jodal:py3/pathlib branch from aaa5bcf to 47d09f8 Oct 8, 2019
@jodal jodal mentioned this pull request Oct 8, 2019
mopidy/config/__init__.py Outdated Show resolved Hide resolved
mopidy/config/__init__.py Outdated Show resolved Hide resolved
mopidy/file/library.py Show resolved Hide resolved
def _find_worker(relative, follow, done, work, results, errors):
"""Worker thread for collecting stat() results.
:param str relative: directory to make results relative to

This comment has been minimized.

Copy link
@kingosticks

kingosticks Oct 8, 2019

Member

Shouldn't relative be a Path?

continue

if relative:
path = os.path.relpath(entry, relative)

This comment has been minimized.

Copy link
@kingosticks

kingosticks Oct 8, 2019

Member

Use Pathlib for all this stuff?

This comment has been minimized.

Copy link
@jodal

jodal Oct 8, 2019

Author Member

This module isn't really ported to pathlib yet. It is used by test_scan.py and the Mopidy-Local scan command. Question is if both this and the audio scanner should be moved to Mopidy-Local.

@adamcik Any thoughts on where this belongs?

Tries to protect against sym/hardlink loops by keeping an eye on parent
(st_dev, st_ino) pairs.
:param str root: root directory to search from, may not be a file

This comment has been minimized.

Copy link
@kingosticks

kingosticks Oct 8, 2019

Member

As above

mopidy/internal/path.py Show resolved Hide resolved
@jodal jodal force-pushed the jodal:py3/pathlib branch from 47d09f8 to bb375d9 Oct 10, 2019
@jodal

This comment has been minimized.

Copy link
Member Author

jodal commented Oct 10, 2019

I've updated the commits here to address all comments, except those related to the three todo items in the PR description.

mopidy/file/library.py Outdated Show resolved Hide resolved
tests/audio/test_scan.py Outdated Show resolved Hide resolved
@jodal jodal force-pushed the jodal:py3/pathlib branch from bb375d9 to 5d20f02 Oct 11, 2019
@kingosticks

This comment has been minimized.

Copy link
Member

kingosticks commented Oct 13, 2019

The tests don't seem very happy here.

@jodal

This comment has been minimized.

Copy link
Member Author

jodal commented Oct 13, 2019

No, the last commit is still WIP. Pushed in case I had any time to continue while away for the weekend.

@kingosticks

This comment has been minimized.

Copy link
Member

kingosticks commented Oct 13, 2019

Ahh! sorry.

On a different subject, I was looking through it again and there are still a few cases where we are logging uris with %s which isn't PY2 friendly. In these cases I don't think %r makes sense, what do you think about adding a helper method on models.fields.String that just gives us unicode for these logging cases?

EDIT:
Or even overriding __ str __?

return list(self._get_media_dirs_refs())

if not self._is_in_basedir(os.path.realpath(local_path)):
if not self._is_in_basedir(local_path):
logger.warning(
'Rejected attempt to browse path (%s) outside dirs defined '

This comment has been minimized.

Copy link
@kingosticks

kingosticks Oct 13, 2019

Member

Example of logging a uri with %s.

This comment has been minimized.

Copy link
@jodal

jodal Oct 18, 2019

Author Member

We're logging an URI, not a path here, so I think we should be good.

This comment has been minimized.

Copy link
@kingosticks

kingosticks Oct 18, 2019

Member

My concern was that a py2 uri is bytes, which may contain non-ASCII, which is mixed here with a unicode literal.

This comment has been minimized.

Copy link
@jodal

jodal Oct 18, 2019

Author Member

I see that. URIs should be URI-encoded, and if so, only contains ASCII. Of course, theory does not equal practise.

One way to solve this is to just move ahead towards Py3, and get rid of Py2.

@jodal jodal force-pushed the jodal:py3/pathlib branch from 5d20f02 to 605d462 Oct 18, 2019
@jodal jodal merged commit 9a8fe43 into mopidy:develop Oct 18, 2019
3 checks passed
3 checks passed
codecov/patch 86.44% of diff hit (target 82.12%)
Details
codecov/project Absolute coverage decreased by -0.05% but relative coverage increased by +4.31% compared to 68ba9f0
Details
test Workflow: test
Details
@jodal jodal deleted the jodal:py3/pathlib branch Oct 18, 2019
kingosticks added a commit to kingosticks/mopidy that referenced this pull request Oct 26, 2019
This was broken in mopidy#1814 but not noticed since the tests were
skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.