mopidy / mopidy-local

Mopidy extension for playing music from your local music archive
https://mopidy.com/ext/local/
Apache License 2.0
61 stars 26 forks source link

Remove code for creating URIs from Musicbrainz IDs #31

Closed fatg3erman closed 4 years ago

fatg3erman commented 4 years ago

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

Fixes #26

codecov[bot] commented 4 years ago

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%) :arrow_up:
mopidy_local/storage.py 45.14% <0%> (+0.51%) :arrow_up:

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 commented 4 years ago

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 commented 4 years ago

What failure are you talking about?

kingosticks commented 4 years ago

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 commented 4 years ago

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.

fatg3erman commented 4 years ago

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.