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

Added an included_file_extensions configuration: closes #8 #32

Closed jessesheidlower closed 4 years ago

jessesheidlower commented 4 years ago

Created separate functions for the file checks, to allow for separate testing/profiling (which I did not do, however). No specific unit tests, but I tried this with many scenarios in my local deployment, and it seems to work.

Please be gentle, this is my first contribution to a Python project!

codecov[bot] commented 4 years ago

Codecov Report

Merging #32 into master will decrease coverage by 0.86%. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
- Coverage   51.12%   50.25%   -0.87%     
==========================================
  Files           9        9              
  Lines         757      770      +13     
==========================================
  Hits          387      387              
- Misses        370      383      +13
Impacted Files Coverage Δ
mopidy_local/commands.py 0% <0%> (ø) :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...f95d239. Read the comment docs.

jodal commented 4 years ago

One more thing: Would be nice with a changelog update together with the change, so the changelog always is up to date and ready for release :-)

jodal commented 4 years ago

And, by running tox locally, you run the project through approximately the same test setup as the CircleCI build which is currently failing. That makes for a lot faster iteration!

kingosticks commented 4 years ago

Sorry I'm late to the review party! Great PR here, my comments are mostly nitpicks.

kingosticks commented 4 years ago

Except this needs to be rebased on master.

jessesheidlower commented 4 years ago

I hope I did the rebase correctly. If so, I think it's ready....

kingosticks commented 4 years ago

Looks like it needs another go through black. It can be annoying until you get in the habit of running tox before pushing.

jessesheidlower commented 4 years ago

Looks like it needs another go through black. It can be annoying until you get in the habit of running tox before pushing.

Yup, got it now. Sorry!
jodal commented 4 years ago

Thank you :-)