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

top_hit is no longer automatically a playlist #175

Closed
2e0byo opened this issue Aug 6, 2023 · 5 comments
Closed

top_hit is no longer automatically a playlist #175

2e0byo opened this issue Aug 6, 2023 · 5 comments

Comments

@2e0byo
Copy link
Collaborator

2e0byo commented Aug 6, 2023

Reviewing one of the recent PRs I found a failing test. Digging into it upstream has changed. Previously we gauranteed (via the test suite) that top_hit would be a playlist if multiple types were passed:

def test_type_search(session):
    search = session.search("Hello", [Playlist, Video])
    assert isinstance(search["top_hit"], Playlist)

    assert len(search["artists"]) == 0
    assert len(search["albums"]) == 0
    assert len(search["tracks"]) == 0
    assert len(search["videos"]) == 50
    assert len(search["playlists"]) == 50

There is no logic for this behaviour in python-tidal and tidal now returns (at least in this case) a video. What should we do?

  1. keep the current behaviour by making a separate request with the type Playlist and then returning its top_hit?
  2. remove the test and drop the behaviour
  3. something else

@tehkillerbee out of interest does this break our browsing 'top hits' in mopidy-tidal? I haven't had time to check.

My preference is for 2, but I don't know if people are relying on this. For now I'll xfail the test.

@tehkillerbee
Copy link
Collaborator

@2e0byo I will test this asap.

@tehkillerbee
Copy link
Collaborator

@2e0byo Just to make sure I understand the issue. When searching for types Playlist and Video, we get results in the opposite order, i.e. the top_hit is a Video and not a Playlist?

In any case, Mopidy-Tidal works as expected when searching so I don't think it is a problem.

@2e0byo
Copy link
Collaborator Author

2e0byo commented Sep 5, 2023

Exactly, yes. The opposite used to be true.

@tehkillerbee
Copy link
Collaborator

@2e0byo Ok, in that case I also vote for 2.

@tehkillerbee
Copy link
Collaborator

Fixed in #228

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

No branches or pull requests

2 participants