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

Limit search query length to 100 characters - Fixes #5105 #5106

Closed
wants to merge 2 commits into from

Conversation

Explosion-Scratch
Copy link

@Explosion-Scratch Explosion-Scratch commented Feb 11, 2024

Description

Fixes #5105

(...)

To Do

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

@Explosion-Scratch
Copy link
Author

Why is formatting check failing? I didn't change anything to at least 13 of the 14 files it fails on

@arsaboo
Copy link
Contributor

arsaboo commented Feb 12, 2024

Here's another PR #4901 that fixes this issue.

@Explosion-Scratch
Copy link
Author

Here's another PR #4901 that fixes this issue.

Yours would still crash if 2 artists names' were longer than 25 characters

@arsaboo
Copy link
Contributor

arsaboo commented Feb 13, 2024

Where did you get 25 characters from? Instead of randomly truncating at 100 characters, searching for two artists is a better idea. Can you try that PR and see if it fixes the issue for you?

@Explosion-Scratch
Copy link
Author

Explosion-Scratch commented Feb 14, 2024

Where did you get 25 characters from? Instead of randomly truncating at 100 characters, searching for two artists is a better idea. Can you try that PR and see if it fixes the issue for you?

25 + 25 = 50 (not sure why you went for 50):

beets/beetsplug/spotify.py

Lines 556 to 557 in a197d4c

if len(artist) > 50:
artist = ",".join(artist.split(",")[:2])

This PR would fix the issue most of the time but there could be cases when this error still occurs. If you added a trunctuate to 100 characters in addition to your PR it would be good I think

@Serene-Arc
Copy link
Contributor

Why is formatting check failing? I didn't change anything to at least 13 of the 14 files it fails on

Black updated their formatter and it changed some lines. The issue has been resolved and didn't just affect your PR. I have pinned the versions in our CI pipeline so it doesn't happen again.

@Serene-Arc
Copy link
Contributor

I prefer @arsaboo's solution to this. Truncating the search randomly has a greater chance of making a meaningless/nonsensical search and thus wrong or no results. Better to properly get at least some of the artists. However some work needs to be done on that PR as well. If you'd like to move to #4901 we can continue discussion.

@Serene-Arc Serene-Arc closed this Mar 1, 2024
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.

Spotify 400 totally stops import
3 participants