Skip to content

Minor fixes for Plex media browser#39878

Merged
balloob merged 6 commits intohome-assistant:devfrom
jjlawren:plex_set_media_class
Oct 1, 2020
Merged

Minor fixes for Plex media browser#39878
balloob merged 6 commits intohome-assistant:devfrom
jjlawren:plex_set_media_class

Conversation

@jjlawren
Copy link
Copy Markdown
Contributor

@jjlawren jjlawren commented Sep 10, 2020

Proposed change

Fix logging message and a trivial cleanup.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

Comment thread homeassistant/components/plex/media_browser.py Outdated
Comment thread homeassistant/components/plex/media_browser.py Outdated
@jjlawren jjlawren force-pushed the plex_set_media_class branch from 1934631 to 33417c1 Compare September 24, 2020 21:36
@jjlawren jjlawren marked this pull request as ready for review September 24, 2020 21:37
@jjlawren
Copy link
Copy Markdown
Contributor Author

The uncovered lines are for failsafes mostly added in other PRs to prevent unexpected media types from passing through. The types provided by Plex are very consistent so I'm not worried about this. Maybe it's safe enough to remove those checks instead of writing tests for events that will never occur...

Comment thread homeassistant/components/plex/media_browser.py Outdated
Comment thread homeassistant/components/plex/media_browser.py Outdated
f"Media not found: {media_content_type} / {media_content_id}"
) from err
_LOGGER.debug("Unknown type received: %s", library_or_section.TYPE)
raise UnknownMediaType from err
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should still raise BrowseError as that's what we're catch when calling this function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call, ran through this too quickly. Luckily we should never receive this error unless Plex changes their API payloads.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it's an impossible case we can remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll dig a bit more first and open a separate PR if it makes sense.

@jjlawren
Copy link
Copy Markdown
Contributor Author

Well, now that most of the changes have been reverted I'm wondering if this did anything helpful in the first place. I believe it may have been based on a misunderstanding on my part while the frontend was still being built. In practice the folder icons all appear to display correctly now in my testing.

@MartinHjelmare
Copy link
Copy Markdown
Member

Update the PR title and description and we can merge.

@jjlawren jjlawren changed the title Plex set media class for consistent directories Minor fixes for Plex media browser Sep 30, 2020
@balloob balloob merged commit cf785b8 into home-assistant:dev Oct 1, 2020
@jjlawren jjlawren deleted the plex_set_media_class branch May 31, 2022 17:52
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.

4 participants