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

Support musicbrainz fields in search and distinct #43

Closed kingosticks closed 4 years ago

kingosticks commented 4 years ago

A fix for #42. My first attempt at using SQLite, hope I did the migration correctly.

This requires https://github.com/mopidy/mopidy/pull/1900

codecov[bot] commented 4 years ago

Codecov Report

Merging #43 into master will not change coverage. The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master      #43   +/-   ##
=======================================
  Coverage   50.39%   50.39%           
=======================================
  Files           9        9           
  Lines         764      764           
=======================================
  Hits          385      385           
  Misses        379      379

Impacted Files	Coverage Δ
mopidy_local/schema.py	`76.28% <50.00%> (ø)`

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 9b38637...ed5ddfd. Read the comment docs.

tkem commented 4 years ago

@kingosticks: I'm pretty impressed you could actually make some sense out of this mess ;-) However, I think you should also update sql/schema.sql, too, for fresh installs. AFAICS, only sql/upgrade-v6.sql has been changed. So with a fresh install, the v5 schema will be created with schema.sql, and then the upgrade-v6.sql script will be run. I always thought it would be more clear if schema.sql contains the current schema. Just easier by looking at a single file to see what's going on. But it's debatable, since that means some code duplication in the upgrade scripts.

tkem commented 4 years ago

BTW, since v5 was the first sqlite schema version used in mopidy-local, the older upgrade scripts could probably be dropped. I guess I should have done this when merging mopidy-local-sqlite in the first place, and maybe reset the schema version to v1.

kingosticks commented 4 years ago

Ahh, I didn't notice that sql/schema.sql actually matched the latest schema (the version number should have been a giveaway). That all makes sense. I've also made those suggested changes.