Repository: fennekki/cdparacord

Issue:
Could not find "parameters".
<!--
Describe your issue and what you'd like to be done about it quickly. If you
intend to implement the solution yourself, you don't need to file a bug before
a pull request.

Note that the maintainer will only take note of issues added to the "continuous
backlog" project. Click "Projects -> continous backlog" on the right hand side
of the issue box and correctly tag your issue as "bug", "enhancement" or
"question" depending on its purpose. This way it'll be visible in the work
queue.
-->
Under some quite unknown circumstances, with at least one specific album (lnTlccHq6F8XNcvNFafPUyaw1mA-), a fresh git clone fails to encode on the first track and crashes.

The script correctly rips track 1 and when it starts to encode there's an issue. After the rip...

Prints:
```
Could not find "parameters".
Can't init infile 'parameters'
```

And then raises:
```
File "cdparacord/rip.py", line 104, in _encode_track
    raise RipError('Failed to encode track {}'.format(track.filename))
```



Discussion:

Patch:
diff --git a/cdparacord/config.py b/cdparacord/config.py
index 962327b..c7e52e3 100644
--- a/cdparacord/config.py
+++ b/cdparacord/config.py
@@ -30,13 +30,11 @@ class Config:
     __default_config = {
         # Config for the encoder
         'encoder': {
-            'lame': {
-                'parameters': [
-                    '-V2',
-                    '${one_file}',
-                    '${out_file}'
-                ]
-            }
+            'lame': [
+                '-V2',
+                '${one_file}',
+                '${out_file}'
+            ]
         },
         # Tasks follow the format of encoder
         # post_rip are run after an individual file has been ripped to a
diff --git a/cdparacord/dependency.py b/cdparacord/dependency.py
index 9f47b51..b2f165e 100644
--- a/cdparacord/dependency.py
+++ b/cdparacord/dependency.py
@@ -27,7 +27,7 @@ class Dependency:
             # dir. I don't think anyone wants that but they... might...
             return name
 
-        for path in os.environ["PATH"].split(os.pathsep):
+        for path in os.environ['PATH'].split(os.pathsep):
             path = path.strip('"')
             binname = os.path.join(path, name)
             if os.path.isfile(binname) and os.access(binname, os.X_OK):
@@ -35,18 +35,53 @@ class Dependency:
 
         # If we haven't returned, the executable was not found
         raise DependencyError(
-            "Executable {} not found or not executable".format(name))
+            'Executable {} not found or not executable'.format(name))
+
+    def _verify_action_params(self, action):
+        """Confirm certain things about action configuration."""
+        if len(action) > 1:
+            multiple_actions = ', '.join(action.keys())
+            raise DependencyError(
+                'Tried to configure multiple actions in one dict: {}'
+                    .format(multiple_actions))
+
+        if len(action) < 1:
+            raise DependencyError(
+                'Configuration opened an action dict but it had no keys')
+
+        action_key = list(action.keys())[0]
+        action_params = action[action_key]
+
+        if type(action_params) is not list:
+            raise DependencyError(
+                '{} configuration has type {} (list expected)'
+                    .format(action_key, type(action_params).__name__))
+
+        for item in action_params:
+            if type(item) is not str:
+                raise DependencyError(
+                    'Found {} parameter {} with type {} (str expected)'
+                        .format(action_key, item, type(item).__name__))
 
     def _discover(self):
         """Discover dependencies and ensure they exist."""
 
-        # Find the executables
+        # Find the executables, and verify parameters for post-actions
+        # and encoder
         self._encoder = self._find_executable(
             list(self._config.get('encoder').keys())[0])
+
+        self._verify_action_params(self._config.get('encoder'))
+
         self._editor = self._find_executable(self._config.get('editor'))
         self._cdparanoia = self._find_executable(
             self._config.get('cdparanoia'))
 
+        for post_action in ('post_rip', 'post_encode', 'post_finished'):
+            for action in self._config.get(post_action):
+                self._find_executable(list(action.keys())[0])
+                self._verify_action_params(action)
+
         # Ensure discid is importable
         try:
             import discid
@@ -54,7 +89,7 @@ class Dependency:
         # it would be ridiculous as it only depends on documented
         # behaviour and only raises a further exception.
         except OSError as e:  # pragma: no cover
-            raise DependencyError("Could not find libdiscid") from e
+            raise DependencyError('Could not find libdiscid') from e
 
     @property
     def encoder(self):


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 0bc761c..1a85588 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -185,4 +185,40 @@ def test_update_config_unknown_keys(mock_temp_home, capsys):
     c.update({'invalid_key': True}, quiet_ignore=False)
 
     out, err = capsys.readouterr()
-    assert err == "Warning: Unknown configuration key invalid_key\n"
+    assert err == 'Warning: Unknown configuration key invalid_key\n'
+
+def test_ensure_default_encoder_keys_are_strings(mock_temp_home):
+    """Test default encoder configuration."""
+    from cdparacord import config
+
+    c = config.Config()
+    
+    assert len(c.get('encoder')) == 1
+
+    for encoder in c.get('encoder'):
+        encoder_params = c.get('encoder')[encoder]
+        # If it's not a list something's wrong
+        assert type(encoder_params) is list
+
+        for item in encoder_params:
+            # And the params should be strings
+            assert type(item) is str
+
+def test_ensure_default_postaction_keys_are_strings(mock_temp_home):
+    """Test default encoder configuration."""
+    from cdparacord import config
+
+    c = config.Config()
+
+    for post_action in ('post_rip', 'post_encode', 'post_finished'):
+        for action in c.get(post_action):
+            assert len(action) == 1
+
+            for action_key in action:
+                action_params = action[action_key]
+                # If it's not a list something's wrong
+                assert type(action_params) is list
+
+                for item in action_params:
+                    # And the params should be strings
+                    assert type(item) is str
diff --git a/tests/test_dependency.py b/tests/test_dependency.py
index 96c0519..e7e92df 100644
--- a/tests/test_dependency.py
+++ b/tests/test_dependency.py
@@ -9,45 +9,57 @@ from cdparacord.dependency import Dependency, DependencyError
 
 
 @pytest.fixture
-def mock_external_binary():
+def mock_config_external():
+    """Mock the Config class such that it returns self.param.
+
+    In addition, querying for  encoder results in a dict that contains
+    an empty list in the key self.param.
+    """
+    class MockConfig:
+        def __init__(self, param):
+            self.param = param
+            self.encoder = {self.param: []}
+            self.post = [{self.param: []}]
+
+        def get(self, name):
+            # Maybe we should write a fake config file but there are
+            # Huge issues with mocking the config module...
+            if name == 'encoder':
+                return self.encoder
+            if name in ('post_rip', 'post_encode', 'post_finished'):
+                return self.post
+            return self.param
+    return MockConfig
+
+
+@pytest.fixture
+def mock_external_encoder(mock_config_external):
     """Mock an external dependency binary.
-    
+
     Create a file, set it to be executable and return it as an
-    ostensible external binary.
+    ostensible external binary via configuration.
     """
     with NamedTemporaryFile(prefix='cdparacord-unittest-') as f:
         os.chmod(f.name, stat.S_IXUSR)
-        yield f.name
 
+        conf = mock_config_external(f.name)
+        yield conf
 
-@pytest.fixture
-def mock_config_external():
-    """Mock the Config class such that it always returns given name."""
-    def get_config(mockbin):
-        class MockConfig:
-            def get(self, name):
-                # Maybe we should write a fake config file but there are
-                # Huge issues with mocking the config module...
-                if name == 'encoder':
-                    return {mockbin: []}
-                return mockbin
-        return MockConfig()
-    return get_config
-
-
-def test_find_valid_absolute_dependencies(mock_external_binary, mock_config_external):
+
+def test_find_valid_absolute_dependencies(mock_external_encoder):
     """Finds fake dependencies that exist by absolute path."""
-    
-    Dependency(mock_config_external(mock_external_binary))
+    Dependency(mock_external_encoder)
 
 
-def test_find_valid_dependencies_in_path(mock_external_binary, mock_config_external, monkeypatch):
+def test_find_valid_dependencies_in_path(mock_external_encoder, monkeypatch):
     """Finds fake dependencies that exist in $PATH."""
 
-    dirname, basename = os.path.split(mock_external_binary)
+    dirname, basename = os.path.split(mock_external_encoder.param)
     # Set PATH to only contain the directory our things are in
     monkeypatch.setenv("PATH", dirname)
-    Dependency(mock_config_external(basename))
+    conf = mock_external_encoder
+    conf.param = basename
+    Dependency(conf)
 
 
 def test_fail_to_find_dependencies(mock_config_external):
@@ -55,28 +67,59 @@ def test_fail_to_find_dependencies(mock_config_external):
         # This file should not be executable by default so the finding
         # should fail
         with pytest.raises(DependencyError):
-            Dependency(mock_config_external(f.name))
+            conf = mock_config_external(f.name)
+            Dependency(conf)
 
 
-def test_get_encoder(mock_config_external, mock_external_binary):
+def test_get_encoder(mock_external_encoder):
     """Get the 'encoder' property."""
 
-    deps = Dependency(mock_config_external(mock_external_binary))
+    deps = Dependency(mock_external_encoder)
     # It's an absolute path so the value should be the same
-    assert deps.encoder == mock_external_binary
+    assert deps.encoder == mock_external_encoder.param
 
 
-def test_get_editor(mock_config_external, mock_external_binary):
+def test_get_editor(mock_external_encoder):
     """Get the 'editor' property."""
 
-    deps = Dependency(mock_config_external(mock_external_binary))
+    deps = Dependency(mock_external_encoder)
     # It's an absolute path so the value should be the same
-    assert deps.editor == mock_external_binary
+    assert deps.editor == mock_external_encoder.param
 
 
-def test_get_cdparanoia(mock_config_external, mock_external_binary):
+def test_get_cdparanoia(mock_external_encoder):
     """Get the 'cdparanoia' property."""
 
-    deps = Dependency(mock_config_external(mock_external_binary))
+    deps = Dependency(mock_external_encoder)
     # It's an absolute path so the value should be the same
-    assert deps.cdparanoia == mock_external_binary
+    assert deps.cdparanoia == mock_external_encoder.param
+
+def test_verify_action_params(mock_external_encoder):
+    """Ensure encoder and post-action parameter verification works."""
+
+    conf = mock_external_encoder
+    deps = Dependency(conf)
+
+    # Dict can only have one key
+    invalid_input = {'a': [], 'b': []}
+    with pytest.raises(DependencyError):
+        deps._verify_action_params(invalid_input)
+
+    # Dict mustn't be empty
+    invalid_input = {}
+    with pytest.raises(DependencyError):
+        deps._verify_action_params(invalid_input)
+
+    # Type of encoder param container should be list
+    invalid_input = {conf.param: {'ah', 'beh'}}
+    with pytest.raises(DependencyError):
+        deps._verify_action_params(invalid_input)
+
+    # Type of encoder param items should be str
+    invalid_input = {conf.param: [1]}
+    with pytest.raises(DependencyError):
+        deps._verify_action_params(invalid_input)
+
+    # Test valid
+    deps._verify_action_params({'valid': ['totally', 'valid']})
+