Repository: mopidy/mopidy-local

Issue:
Put back the use_album_mbid_uri option that was in local-sqlite
Using MBIDs to create uris can have unintended consequences. With local-sqlite I could switch this off but this option seems to have been removed in the new mopidy-local.

As an example, I have 2 albums that I tagged with Musicbrainz Picard.

One has 'Artist = Brian Eno'
One has 'Artist = Brian Eno & Jah Wobble'

They're also tagged with musicbrainz IDs for the artists. The second album has two artist ID tags even though it only has one artist. (This is the way Musicbrainz reccomend it is done). Since the scanner can only use one mbid to create the artist URI, both albums end up under 'Brian Eno and Jah Wobble'.

With the old local-sqlite setting use_album_mbid_uri = false in mopidy.conf fixed this.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/mopidy/mopidy-local/pull/31?src=pr&el=h1) Report
> Merging [#31](https://codecov.io/gh/mopidy/mopidy-local/pull/31?src=pr&el=desc) into [master](https://codecov.io/gh/mopidy/mopidy-local/commit/6865fac7254674170d1d2a92e3302dcd10b337da?src=pr&el=desc) will **increase** coverage by `0.34%`.
> The diff coverage is `0%`.

[![Impacted file tree graph](https://codecov.io/gh/mopidy/mopidy-local/pull/31/graphs/tree.svg?width=650&token=rEF2NwRisd&height=150&src=pr)](https://codecov.io/gh/mopidy/mopidy-local/pull/31?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
+ Coverage   51.12%   51.46%   +0.34%     
==========================================
  Files           9        9              
  Lines         757      750       -7     
==========================================
- Hits          387      386       -1     
+ Misses        370      364       -6
```


| [Impacted Files](https://codecov.io/gh/mopidy/mopidy-local/pull/31?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [mopidy\_local/library.py](https://codecov.io/gh/mopidy/mopidy-local/pull/31/diff?src=pr&el=tree#diff-bW9waWR5X2xvY2FsL2xpYnJhcnkucHk=) | `40.15% <ø> (+0.73%)` | :arrow_up: |
| [mopidy\_local/storage.py](https://codecov.io/gh/mopidy/mopidy-local/pull/31/diff?src=pr&el=tree#diff-bW9waWR5X2xvY2FsL3N0b3JhZ2UucHk=) | `45.14% <0%> (+0.51%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/mopidy/mopidy-local/pull/31?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/mopidy/mopidy-local/pull/31?src=pr&el=footer). Last update [6865fac...2c72512](https://codecov.io/gh/mopidy/mopidy-local/pull/31?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

fatg3erman: 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: What failure are you talking about?
kingosticks: 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.
jodal: 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: 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.

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 32bf2bb..9576cfa 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -10,8 +10,9 @@ v3.1.0 (UNRELEASED)
 - Add ``.cue`` to the list of excluded file extensions. (PR: #29)
 - Replace ``os.path`` usage with ``pathlib`` to handle arbitrary file path
   encodings better. (#20, PR: #30)
+- Add an ``included_files_extensions`` config. (#8, PR: #32)
 
-
+  
 v3.0.0 (2019-12-22)
 ===================
 
diff --git a/README.rst b/README.rst
index 6adafaa..e32fe4d 100644
--- a/README.rst
+++ b/README.rst
@@ -91,9 +91,16 @@ The following configuration values are available:
   library it should try and store its progress so far. Some libraries might not
   respect this setting. Set this to zero to disable flushing.
 
+- ``local/included_file_extensions``: File extensions to include when scanning
+  the media directory. Values should be separated by either comma or newline.
+  Each file extension should start with a dot, .e.g. ``.flac``. Setting any
+  values here will override the existence of ``local/excluded_file_extensions``.
+
 - ``local/excluded_file_extensions``: File extensions to exclude when scanning
   the media directory. Values should be separated by either comma or newline.
-  Each file extension should start with a dot, .e.g. ``.html``.
+  Each file extension should start with a dot, .e.g. ``.html``. Defaults to a
+  list of common non-audio file extensions often found in music collections.
+  This config value has no effect if ``local/included_file_extensions`` is set.
 
 - ``local/directories``: List of top-level directory names and URIs
   for browsing.
diff --git a/mopidy_local/__init__.py b/mopidy_local/__init__.py
index d4d881a..225ecd2 100644
--- a/mopidy_local/__init__.py
+++ b/mopidy_local/__init__.py
@@ -26,6 +26,7 @@ class Extension(ext.Extension):
         schema["scan_timeout"] = config.Integer(minimum=1000, maximum=1000 * 60 * 60)
         schema["scan_flush_threshold"] = config.Integer(minimum=0)
         schema["scan_follow_symlinks"] = config.Boolean()
+        schema["included_file_extensions"] = config.List(optional=True)
         schema["excluded_file_extensions"] = config.List(optional=True)
         schema["directories"] = config.List()
         schema["timeout"] = config.Integer(optional=True, minimum=1)
diff --git a/mopidy_local/commands.py b/mopidy_local/commands.py
index 5dc213d..f195614 100644
--- a/mopidy_local/commands.py
+++ b/mopidy_local/commands.py
@@ -81,6 +81,10 @@ class ScanCommand(commands.Command):
                 media_dir=media_dir,
                 file_mtimes=file_mtimes,
                 files_in_library=files_in_library,
+                included_file_exts=[
+                    file_ext.lower()
+                    for file_ext in config["local"]["included_file_extensions"]
+                ],
                 excluded_file_exts=[
                     file_ext.lower()
                     for file_ext in config["local"]["excluded_file_extensions"]
@@ -143,19 +147,56 @@ class ScanCommand(commands.Command):
         return files_to_update, files_in_library
 
     def _find_files_to_scan(
-        self, *, media_dir, file_mtimes, files_in_library, excluded_file_exts
+        self,
+        *,
+        media_dir,
+        file_mtimes,
+        files_in_library,
+        included_file_exts,
+        excluded_file_exts,
     ):
         files_to_update = set()
 
+        def _is_hidden_file(relative_path, file_uri):
+            if any(p.startswith(".") for p in relative_path.parts):
+                logger.debug(f"Skipped {file_uri}: Hidden directory/file")
+                return True
+            else:
+                return False
+
+        def _extension_filters(
+            relative_path, file_uri, included_file_exts, excluded_file_exts
+        ):
+            if included_file_exts:
+                if relative_path.suffix.lower() in included_file_exts:
+                    logger.debug(f"Added {file_uri}: File extension on included list")
+                    return True
+                else:
+                    logger.debug(
+                        f"Skipped {file_uri}: File extension not on included list"
+                    )
+                    return False
+            else:
+                if relative_path.suffix.lower() in excluded_file_exts:
+                    logger.debug(f"Skipped {file_uri}: File extension on excluded list")
+                    return False
+                else:
+                    logger.debug(
+                        f"Included {file_uri}: File extension not on excluded list"
+                    )
+                    return True
+
         for absolute_path in file_mtimes:
             relative_path = absolute_path.relative_to(media_dir)
             file_uri = absolute_path.as_uri()
 
-            if any(p.startswith(".") for p in relative_path.parts):
-                logger.debug(f"Skipped {file_uri}: Hidden directory/file")
-            elif relative_path.suffix.lower() in excluded_file_exts:
-                logger.debug(f"Skipped {file_uri}: File extension excluded")
-            elif absolute_path not in files_in_library:
+            if (
+                not _is_hidden_file(relative_path, file_uri)
+                and _extension_filters(
+                    relative_path, file_uri, included_file_exts, excluded_file_exts
+                )
+                and absolute_path not in files_in_library
+            ):
                 files_to_update.add(absolute_path)
 
         logger.info(f"Found {len(files_to_update)} tracks which need to be updated")
diff --git a/mopidy_local/ext.conf b/mopidy_local/ext.conf
index cbe97df..08e1f85 100644
--- a/mopidy_local/ext.conf
+++ b/mopidy_local/ext.conf
@@ -5,6 +5,7 @@ media_dir = $XDG_MUSIC_DIR
 scan_timeout = 1000
 scan_flush_threshold = 100
 scan_follow_symlinks = false
+included_file_extensions =
 excluded_file_extensions =
   .cue
   .directory
@@ -18,6 +19,7 @@ excluded_file_extensions =
   .txt
   .zip
 
+
 # top-level directories for browsing, as <name> <uri>
 directories =
     Albums                  local:directory?type=album
diff --git a/mopidy_local/library.py b/mopidy_local/library.py
index e77460e..61ae109 100644
--- a/mopidy_local/library.py
+++ b/mopidy_local/library.py
@@ -1,4 +1,3 @@
-import hashlib
 import logging
 import operator
 import sqlite3
@@ -203,9 +202,3 @@ class LocalLibraryProvider(backend.LibraryProvider):
             return [{"album": uri}]
         else:
             return []
-
-    def _model_uri(self, type, model):
-        if model.musicbrainz_id and self._config["use_%s_mbid_uri" % type]:
-            return f"local:{type}:mbid:{model.musicbrainz_id}"
-        digest = hashlib.md5(str(model)).hexdigest()
-        return f"local:{type}:md5:{digest}"
diff --git a/mopidy_local/storage.py b/mopidy_local/storage.py
index d5b28fd..a8679c3 100644
--- a/mopidy_local/storage.py
+++ b/mopidy_local/storage.py
@@ -31,10 +31,7 @@ def get_image_size_gif(data):
 
 
 def model_uri(type, model):
-    # only use valid mbids; TODO: use regex for that?
-    if model.musicbrainz_id and len(model.musicbrainz_id) == 36:
-        return f"local:{type}:mbid:{model.musicbrainz_id}"
-    elif type == "album":
+    if type == "album":
         # ignore num_tracks for multi-disc albums
         digest = hashlib.md5(str(model.replace(num_tracks=None)).encode())
     else:


Test patch:
diff --git a/tests/test_extension.py b/tests/test_extension.py
index e11c4a1..ff79cc2 100644
--- a/tests/test_extension.py
+++ b/tests/test_extension.py
@@ -23,6 +23,7 @@ def test_get_config_schema():
     assert "scan_timeout" in schema
     assert "scan_flush_threshold" in schema
     assert "scan_follow_symlinks" in schema
+    assert "included_file_extensions" in schema
     assert "excluded_file_extensions" in schema
     # from mopidy-local-sqlite
     assert "directories" in schema