search

mopidy / mopidy-beets

Mopidy extension for playing music from a Beets collection
https://mopidy.com/ext/beets/
MIT License
49 stars 14 forks source link

Fix queries for library items containing slashes (/) #36

Open 01tot10 opened 1 year ago

01tot10 commented 1 year ago

In it's current form, beets library items with slashes / in their names are not found correctly. Examples of such items in my library are artists with names such as Harold Budd/Brian Eno or genres such as Ambient/Experimental.

I poked around BeetsRemoteClient in client.py, more specifically the _get_objects_by_attribute-method which makes the queries for Beets Web, and figured that removing the regex search for items containing slashes does the trick.

I'm quite sure the regex search could also be adjusted to include slashes, but I couldn't find a way I was happy with, although I got quite close. Since removing the regex search fixes the functionality, I thought it's already a good step forward!

Let me know if I should adjust the PR somehow before it can be accepted!

01tot10 commented 1 year ago

Running flake8 locally results in warnings caused by the various @cache()-statements, but since those were added earlier and have nothing to do with this PR, I ignored them.

01tot10 commented 1 year ago

Hey @jodal! Could you help out with these two PRs of mine (the other is https://github.com/mopidy/mopidy-beets/pull/37)? They both get stuck at flake8 complaining about lines in the existing code base that I haven't touched, and I feel uncertain how to proceed. Should I try to look into the existing code and fix the flake8 warnings on a separate PR, or can we somehow ignore the warnings since they are not a result of something I did, and proceed with merging?

kingosticks commented 1 year ago

If you could fix it up in your PR that'd be good. It fails, despite you not touching these lines, because the version of flake8 has changed compared to when we last ran CI (and also compared to whatever version you ran locally).

The fix is at https://github.com/mopidy/mopidy-soundcloud/pull/132/files

01tot10 commented 1 year ago

Thanks a lot @kingosticks, that was really helpful! I added the suggested edits to setup.cfg

kingosticks commented 1 year ago

If you run black . it'll make the formatting changes for you. You can run these same linting steps locally if you want, tox --recreate is good for this as it recreates the virtual env and ensures the latest versions of flake8 and black are used.

For the PR itself, I don't use beets (and we don't have any tests!) so I'm not well placed to merge this. Maybe @sumpfralle can help.

01tot10 commented 1 year ago

Thanks again, and sorry for the extra trouble. This is the first time I'm dealing with automated CI/CD loops. Hmm, I have been running black to format my code, and so far I thought I'm respecting the guidelines.

> black .
All done! ✨ 🍰 ✨
10 files left unchanged.
> git status
On branch fix/slash-queries
Your branch is up to date with '01tot10/fix/slash-queries'.
nothing to commit, working tree clean
> tox -e flake8
... (long list of version numbers)
flake8: commands succeeded
congratulations :)

I didn't know of tox --recreate, thanks for the tip. Running tox --recreate instead of tox to run the tests doesn't show anything different though:

tox --recreate
... (tests)
  py37: commands succeeded
  py38: commands succeeded
ERROR:  py39: InterpreterNotFound: python3.9
  check-manifest: commands succeeded
  flake8: commands succeeded

I don't have a python3.9 intepreter, but that didn't seem to fail during the integration loop, whereas flake8 doesn't show any errors. Am I missing something?

kingosticks commented 1 year ago

Sorry, seems I've misremembered and thought --recreate installed latest versions. I've just fallen into the same trap i always do.

You'll have to manually upgrade your local versions, pip install --upgrade black flake8. The versions in your local run need to match the latest ones that are used in CI. Sorry about that, it can be quite annoying chasing these down.

01tot10 commented 1 year ago

No need to be sorry, I really appreciate your help and patience! In some sense it has been illuminating having these extra complications, since it has also thought me many new things I haven't been aware of!

Anyways, I upgraded black and flake8 in my environment, and was able to have the linter complaining about the formatting, as did the CI loop. I fixed the formatting in an extra commit. Fingers crossed 🤞🏻.