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

Merging mopidy-local-sqlite and mopidy-local-images into mopidy-local #10

Merged
merged 23 commits into from
Dec 9, 2019
Merged

Conversation

tkem
Copy link
Member

@tkem tkem commented Sep 12, 2019

My initial attempt, don't judge too harshly ;-)
I split up the Local library into LocalLibraryProvider and LocalStorageProvider, the latter being used by mopidy local scan only. Otherwise, this is pretty straight-forward, no attempt at cleaning things up except for getting rid of some config settings that (probably) nobody used, anyway.
I did some very basic testing, and it seems to basically work. One thing I noticed was that when scanning a large (~20,000 tracks) library, mopidy local scan uses several GB of memory. That's something we should investigate further.

@jodal
Copy link
Member

jodal commented Sep 12, 2019

❤️

@tkem
Copy link
Member Author

tkem commented Dec 4, 2019

Updated according to https://gist.github.com/jodal/6c011e327035cc4ad56dbc2e3c28f1eb (to my best knowledge) and tested basic functionality with Mopidy 3.0.0a6.
Probably still lots of bugs, also disabled some unit tests for now, most noticeably test_translator.py.
I'd especially welcome input on translator.py, i.e. using pathlib for handling file paths, etc.
Note that the README still needs to be updated with new config parameters, etc.

README.rst Outdated Show resolved Hide resolved
uris_to_remove.add(track.uri)
elif mtime > track.last_modified or args.force:
uris_to_update.add(track.uri)
uris_in_library.add(track.uri)

logger.info('Removing %d missing tracks.', len(uris_to_remove))
logger.info("Removing %d missing tracks.", len(uris_to_remove))
for uri in uris_to_remove:
library.remove(uri)

for abspath in file_mtimes:
relpath = os.path.relpath(abspath, media_dir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use Pathlib here and all through this next section:

relpath = abspath.relative_to(media_dir)

relpath = relpath.encode('utf-8')
return 'local:track:%s' % urllib.quote(relpath)
if isinstance(relpath, str):
relpath = relpath.encode("utf-8")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "utf-8" argument is at least redundant.

If relpath can contain "surrogateescape" encoded random bytes, which is the sane way to pass random bytes through Unicode text, this, and all other places where paths are encoded, should be changed to:

relpath = relpath.encode(errors="surrogateescape")

Copy link
Member

@jodal jodal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! I'm comfortable merging this and doing any further improvement iteratively on the master branch, with some preleases inbetween the improvements 👍

CHANGELOG.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
mopidy_local/__init__.py Show resolved Hide resolved
mopidy_local/commands.py Show resolved Hide resolved
mopidy_local/commands.py Show resolved Hide resolved
mopidy_local/library.py Show resolved Hide resolved

logger = logging.getLogger(__name__)


def check_dirs_and_files(config):
if not os.path.isdir(config['local']['media_dir']):
if not os.path.isdir(config["local"]["media_dir"]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from pathlib import Path

if not Path(config["local"]["media_dir"]).is_dir():
    ...

Comment on lines +123 to +127
for root, dirs, files in os.walk(self._image_dir, topdown=False):
for name in dirs:
os.rmdir(os.path.join(root, name))
for name in files:
os.remove(os.path.join(root, name))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be replaced with shutil.rmtree()?

relpath = relpath.encode('utf-8')
return 'local:track:%s' % urllib.quote(relpath)
if isinstance(relpath, str):
relpath = relpath.encode("utf-8")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "utf-8" argument is at least redundant.

If relpath can contain "surrogateescape" encoded random bytes, which is the sane way to pass random bytes through Unicode text, this, and all other places where paths are encoded, should be changed to:

relpath = relpath.encode(errors="surrogateescape")

mopidy_local/www/index.html Show resolved Hide resolved
@tkem
Copy link
Member Author

tkem commented Dec 9, 2019

@jodal, @kingosticks: Thanks for reviewing! I don't know how much time I can spend on this short-term, so since this basically seems to work (at least for me), I'd also suggest just merging this and doing the rest iteratively...

@jodal jodal merged commit d6cdd76 into mopidy:master Dec 9, 2019
@jodal jodal added this to the v3.0.0 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants