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

Ignore parts of web responses with a None URI. #290

Merged
merged 1 commit into from
Dec 6, 2020

Conversation

kingosticks
Copy link
Member

@kingosticks kingosticks commented Dec 4, 2020

We can't do anything sensible without a URI so also ignore anything where it is None. This occurs in the album and artist fields within local Spotify tracks (from a playlist).

Without this we try and translate the artist and album fields, which isn't useful and in the case of changes in #287 actually causes an exception.

@kingosticks kingosticks self-assigned this Dec 4, 2020
@kingosticks kingosticks added A-webapi Area: Spotify Web API C-bug Category: This is a bug labels Dec 4, 2020
@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #290 (a4aa63a) into master (915cf34) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #290   +/-   ##
=======================================
  Coverage   96.68%   96.68%           
=======================================
  Files          13       13           
  Lines        1206     1206           
=======================================
  Hits         1166     1166           
  Misses         40       40           
Impacted Files Coverage Δ
mopidy_spotify/translator.py 96.33% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 915cf34...a4aa63a. Read the comment docs.

@kingosticks
Copy link
Member Author

It would be preferable to remove all None values from the a web response but seeing as we don't go near half the fields that seems a lot of extra work.

@kingosticks kingosticks requested a review from jodal December 5, 2020 23:35
@kingosticks kingosticks force-pushed the fix/web-types-with-none-uri branch from 23b28f1 to 29ebee6 Compare December 5, 2020 23:37
Copy link
Member

@jodal jodal left a comment

Choose a reason for hiding this comment

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

LGTM, with the single comment.

We can't do anything sensible without a URI so ignore anything that
doesn't have one. This occurs in the album and artist fields within
local Spotify tracks (from a playlist).
@kingosticks kingosticks force-pushed the fix/web-types-with-none-uri branch from 29ebee6 to a4aa63a Compare December 6, 2020 00:44
@kingosticks kingosticks merged commit 778099d into mopidy:master Dec 6, 2020
@kingosticks kingosticks deleted the fix/web-types-with-none-uri branch December 6, 2020 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-webapi Area: Spotify Web API C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants