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

search: adds functionality to remove search suggestion #678

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

Conversation

comarquet
Copy link

Related to #497

I modified parse_search_suggestions so that it retrieves the feedbackToken of each suggestion in the search bar and associates it with the number it is displayed when invoking get_search_suggestions, so that the function remove_search_suggestion can be called with the number as parameter to delete the corresponding entry.

image

Copy link
Owner

@sigma67 sigma67 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the contribution! And you even added tests :)

Looks pretty good, some minor comments and suggestions below.

@@ -7,6 +7,10 @@


class SearchMixin(MixinProtocol):
def __init__(self):
self._latest_suggestions = None
self._latest_feedback_tokens = None
Copy link
Owner

Choose a reason for hiding this comment

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

Despite being an instance, we don't store request-specific state in the YTMusic instance between function calls.

The only state in YTMusic relates to authentication/configuration.

Please move the variables into function parameters.

@@ -295,7 +299,8 @@ def get_search_suggestions(self, query: str, detailed_runs=False) -> Union[list[
{
"text": "d"
}
]
],
"number": 1
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer index, starting at 0


return search_suggestions

def remove_search_suggestion(self, number: int) -> bool:
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, please rename number to index

print("Failed to remove suggestion")
"""
if self._latest_suggestions is None or self._latest_feedback_tokens is None:
raise ValueError(
Copy link
Owner

Choose a reason for hiding this comment

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

Please raise YTMusicError or YTMusicUserError as you see fit

ytmusicapi/mixins/search.py Outdated Show resolved Hide resolved
ytmusicapi/parsers/search.py Outdated Show resolved Hide resolved
ytmusicapi/parsers/search.py Outdated Show resolved Hide resolved
ytmusicapi/parsers/search.py Outdated Show resolved Hide resolved
tests/mixins/test_search.py Show resolved Hide resolved
@comarquet
Copy link
Author

Hi ! Thank you for your feedback; it’s greatly appreciated. I’ve implemented all the changes you suggested.

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