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

feat: Add sonicAdventure method to MusicSection #1361

Merged
merged 12 commits into from
Mar 16, 2024

Conversation

Dr-Blank
Copy link
Contributor

@Dr-Blank Dr-Blank commented Feb 11, 2024

Fixes #1360

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the docstring for new or existing methods
  • I have added tests when applicable

@Dr-Blank
Copy link
Contributor Author

not sure if I understand why the tests failed for this change

Copy link
Collaborator

@JonnyWong16 JonnyWong16 left a comment

Choose a reason for hiding this comment

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

I believe sonic adventure is a Plex Pass feature so it will need to be an authenticated test.

It might also be useful to add a convenience method to tracks.

def sonicAdventure(self, to):
    self.section().sonicAdventure(start=self, end=to)
>>> track1.sonicAdventure(to=track2)

plexapi/library.py Outdated Show resolved Hide resolved
@Dr-Blank
Copy link
Contributor Author

I believe sonic adventure is a Plex Pass feature so it will need to be an authenticated test.

It might also be useful to add a convenience method to tracks.

def sonicAdventure(self, to):
    self.section().sonicAdventure(start=self, end=to)
>>> track1.sonicAdventure(to=track2)

I did not add this in this PR as to not clutter it like I have a habit to, which makes it more cumbersome to review. was holding it off for once this was merged/approved

plexapi/audio.py Outdated Show resolved Hide resolved
@@ -398,6 +399,14 @@ def test_audio_Track_lyricStreams(track):
assert not track.lyricStreams()


@pytest.mark.authenticated
def test_audio_Track_sonicAdventure(music):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these tests work on the bootstrap test server? It only has one track and we disable all the analysis tasks (although it looks like we forgot to disable sonic analysis).

# Disable settings for background tasks when using the test server.
# These tasks won't work on the test server since we are using fake media files

Also needs a Plex Pass account.

Suggested change
def test_audio_Track_sonicAdventure(music):
def test_audio_Track_sonicAdventure(account_plexpass, music):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it might not, I changed my bootstrap server by adding a few tracks from my collection to test these features

Copy link
Collaborator

Choose a reason for hiding this comment

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

So this does not work on the bootstrap server with a single track, or even two tracks.

The tests fail at assert len(adventure) because there are no computed tracks. I think we just check if it is a list assert isinstance(adventure, list) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this does not work on the bootstrap server with a single track, or even two tracks.

would return list of 2 tracks, same as the args if not adventure between them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried this on my own server and it returns a blank list. The start and end are not included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I would be wrong on this then, however the tests should be okay regardless

tests/test_library.py Outdated Show resolved Hide resolved
plexapi/library.py Outdated Show resolved Hide resolved
Dr-Blank and others added 4 commits February 18, 2024 04:05
- still asserts it is an iterable
- still checks all elements are tracks
- in case no adventure found, does not fail
@JonnyWong16 JonnyWong16 merged commit 7802b79 into pkkid:master Mar 16, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Endpoint for computing sonic adventure
3 participants