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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

01tot10
Copy link

@01tot10 01tot10 commented Jan 6, 2023

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
Copy link
Author

01tot10 commented Jan 17, 2023

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
Copy link
Author

01tot10 commented Feb 24, 2023

Hey @jodal! Could you help out with these two PRs of mine (the other is #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
Copy link
Member

kingosticks commented Feb 24, 2023

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
Copy link
Author

01tot10 commented Feb 25, 2023

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

@kingosticks
Copy link
Member

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
Copy link
Author

01tot10 commented Feb 25, 2023

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
Copy link
Member

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
Copy link
Author

01tot10 commented Feb 26, 2023

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 🤞🏻.

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.

2 participants