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

Remove code for creating URIs from Musicbrainz IDs #31

Merged
merged 3 commits into from
Jan 9, 2020
Merged

Remove code for creating URIs from Musicbrainz IDs #31

merged 3 commits into from
Jan 9, 2020

Conversation

fatg3erman
Copy link
Contributor

Remove any code that was used for creating URIs from MBIDs.

Fixes #26

@codecov
Copy link

codecov bot commented Jan 5, 2020

Codecov Report

Merging #31 into master will increase coverage by 0.27%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #31      +/-   ##
=========================================
+ Coverage   51.12%   51.4%   +0.27%     
=========================================
  Files           9       9              
  Lines         757     749       -8     
=========================================
- Hits          387     385       -2     
+ Misses        370     364       -6
Impacted Files Coverage Δ
mopidy_local/library.py 39.69% <ø> (+0.27%) ⬆️
mopidy_local/storage.py 45.14% <0%> (+0.51%) ⬆️

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 6865fac...ea5f2ea. Read the comment docs.

@fatg3erman
Copy link
Contributor Author

I don't even know what this failure means.

Honestly, I was considering suggesting myself as maintainer for this, but these checks are unintelligible and ridiculously pedantic.

@kingosticks
Copy link
Member

What failure are you talking about?

@kingosticks
Copy link
Member

kingosticks commented Jan 5, 2020

If you are talking about the codecov failure, then it's basically telling you that you didn't test anything that you changed. The tests for this extension are not brilliant anyway, and it's not going to stop anyone merging this. It would be nice if we could increase the coverage to a point where these failures made more sense but I disagree they are "ridiculously pedantic".

The actual tests themselves passed, for what they are worth in this case.

@jodal
Copy link
Member

jodal commented Jan 6, 2020

The "codecov/project" test tells us that the project as a whole's test coverage is 51.4%, and merging this PR will increase the test coverage with +0.27%.

The "codecov/patch" test, which is the one failing, tells us that the code changed by this PR has 0% test coverage, which is less than the goal of 51.12%, which is the project's test coverage before this PR is merged. In other words, the code changed by the PR has less coverage than the project as a whole. Assuming this PR was adding code, not removing it, that would cause the project as a whole to get less test coverage, which is moving in the right direction.

I tend to view these reports as "extra information" and not something blocking me from merging. I've seen it positively affect new contributors by nudging them to make the extra effort to test their changes better. If we all did that, we'd end up in a great place over time.

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! Would be nice with a changelog update together with the change, so the changelog always is up to date and ready for release :-)

@fatg3erman
Copy link
Contributor Author

As a professional QA engineer for 25 years I'd be the first to agree that nobody does enough testing :)

As for 'ridiculously pedantic' I was more referring to flake8. While I agree with the need for code to be elegant and readable I feel it stretches into nitpicking territory and it annoys the lviing **** out of me. It puts me off contributing more than it helps. But that's just me.

If I get the time I'll update the changelog and commit it.

Copy link
Member

@tkem tkem left a comment

Choose a reason for hiding this comment

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

Looks good to me, too! Thanks!

@jodal jodal added this to the v3.1.0 milestone Jan 8, 2020
@jodal jodal merged commit 9b946c7 into mopidy:master Jan 9, 2020
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.

Put back the use_album_mbid_uri option that was in local-sqlite
4 participants