#133: Code review fixes

Author: shacharoo

docs/release_notes.rst

@@ -22,8 +22,10 @@ New Features
   will now be checked by default instead of D203, which required a single
   blank line before a class docstring (#137).
 
-* Now handles configuration filed correctly. The closer a configuration file
-  is to a checked file the more it matters (#133).
+* Configuration files are now handles correctly. The closer a configuration file
+  is to a checked file the more it matters.
+  Configuration files no longer support ``explain``, ``source``, ``debug``,
+  ``verbose`` or ``count`` (#133).
 
 Bug Fixes
 

docs/snippets/config.rst

@@ -27,9 +27,9 @@ Inheritance
 ###########
 
 By default, when finding a configuration file, ``pep257`` tries to inherit
-the parent directory's configuration and migrate them to the local ones.
+the parent directory's configuration and merge them to the local ones.
 
-The migration process is as follows:
+The merge process is as follows:
 
 * If one of ``select``, ``ignore`` or ``convention`` was specified in the child
   configuration - Ignores the parent configuration and set the new error codes

src/pep257.py

@@ -21,7 +21,7 @@
 from itertools import takewhile, dropwhile, chain
 from re import compile as re
 import itertools
-from collections import defaultdict, namedtuple
+from collections import defaultdict, namedtuple, Set
 
 try:  # Python 3.x
     from ConfigParser import RawConfigParser
@@ -724,7 +724,7 @@ class ConfigurationParser(object):
 
     Configuration files are nested within the file system, meaning that the
     closer a configuration file is to a checked file, the more relevant it will
-    be. For instance, imagine this folder structure:
+    be. For instance, imagine this directory structure:
 
     A
     +-- tox.ini: sets `select=D100`
@@ -735,15 +735,15 @@ class ConfigurationParser(object):
     Then `foo.py` will not be checked for `D100`.
     The configuration build algorithm is described in `self._get_config`.
 
-    Note: If any of `MUTUALLY_EXCLUSIVE_OPTIONS` was selected in the CLI, all
+    Note: If any of `BASE_ERROR_SELECTION_OPTIONS` was selected in the CLI, all
     configuration files will be ignored and each file will be checked for
     the error codes supplied in the CLI.
 
     """
 
     CONFIG_FILE_OPTIONS = ('convention', 'select', 'ignore', 'add-select',
                            'add-ignore', 'match', 'match-dir')
-    MUTUALLY_EXCLUSIVE_OPTIONS = ('ignore', 'select', 'convention')
+    BASE_ERROR_SELECTION_OPTIONS = ('ignore', 'select', 'convention')
 
     DEFAULT_MATCH_RE = '(?!test_).*\.py'
     DEFAULT_MATCH_DIR_RE = '[^\.].*'
@@ -768,7 +768,7 @@ def get_default_run_configuration(self):
     def parse(self):
         """Parse the configuration.
 
-        If one of `MUTUALLY_EXCLUSIVE_OPTIONS` was selected, overrides all
+        If one of `BASE_ERROR_SELECTION_OPTIONS` was selected, overrides all
         error codes to check and disregards any error code related
         configurations from the configuration files.
 
@@ -791,13 +791,16 @@ def get_user_run_configuration(self):
 
     @check_initialized
     def get_files_to_check(self):
-        """Return a generator of files and error codes to check on each file.
+        """Generate files and error codes to check on each one.
 
         Walk dir trees under `self._arguments` and generate yield filnames
         that `match` under each directory that `match_dir`.
         The method locates the configuration for each file name and yields a
         tuple of (filename, [error_codes]).
 
+        With every discovery of a new configuration file `IllegalConfiguration`
+        might be raised.
+
         """
         def _get_matches(config):
             """Return the `match` and `match_dir` functions for `config`."""
@@ -834,25 +837,25 @@ def _get_config(self, node):
 
         The algorithm:
         -------------
-        1.  If the current directory's configuration exists in
-            `self._cache` - return it.
-        2.  If a configuration file does not exist in this directory:
-        3.    If there's a parent directory:
-        4.      Cache it's configuration as this directory's and return it.
-        5.    Else:
-        6.      Cache a default configuration and return it.
-        7.  Else:
-        8.    Read the configuration file.
-        9.    If a parent directory exists AND the configuration file
-              allows inheritance:
-        10.     Read the parent configuration by calling this function with the
-                parent directory as `node`.
-        11.     Migrate the parent configuration with the current one and
-                cache it.
-        12. If the user has specified one of `MUTUALLY_EXCLUSIVE_OPTIONS` in
-            the CLI - return the CLI configuration with the configuration match
-            clauses
-        13. Set the `--add-select` and `--add-ignore` CLI configurations.
+        * If the current directory's configuration exists in
+           `self._cache` - return it.
+        * If a configuration file does not exist in this directory:
+        *   If the directory is not a root directory:
+        *     Cache its configuration as this directory's and return it.
+        *   Else:
+        *     Cache a default configuration and return it.
+        * Else:
+        *   Read the configuration file.
+        *   If a parent directory exists AND the configuration file
+            allows inheritance:
+        *     Read the parent configuration by calling this function with the
+              parent directory as `node`.
+        *     Merge the parent configuration with the current one and
+              cache it.
+        * If the user has specified one of `BASE_ERROR_SELECTION_OPTIONS` in
+          the CLI - return the CLI configuration with the configuration match
+          clauses
+        * Set the `--add-select` and `--add-ignore` CLI configurations.
 
         """
         path = os.path.abspath(node)
@@ -873,16 +876,16 @@ def _get_config(self, node):
                 # Use the default configuration or the one given in the CLI.
                 config = self._create_check_config(self._options)
         else:
-            # There's a config file! Read it and migrate if necessary.
+            # There's a config file! Read it and merge if necessary.
             options, inherit = self._read_configuration_file(config_file)
 
             parent_dir, tail = os.path.split(path)
             if tail and inherit:
-                # There is a parent dir and we should try to migrate.
+                # There is a parent dir and we should try to merge.
                 parent_config = self._get_config(parent_dir)
-                config = self._migrate_configuration(parent_config, options)
+                config = self._merge_configuration(parent_config, options)
             else:
-                # No need to migrate or parent dir does not exist.
+                # No need to merge or parent dir does not exist.
                 config = self._create_check_config(options)
 
         # Make the CLI always win
@@ -949,8 +952,8 @@ def _read_configuration_file(self, path):
 
         return options, should_inherit
 
-    def _migrate_configuration(self, parent_config, child_options):
-        """Migrate parent config into the child options.
+    def _merge_configuration(self, parent_config, child_options):
+        """Merge parent config into the child options.
 
         The migration process requires an `options` object for the child in
         order to distinguish between mutually exclusive codes, add-select and
@@ -1057,18 +1060,18 @@ def _get_checked_errors(cls, options):
 
         cls._set_add_options(checked_codes, options)
 
-        return checked_codes - set('')
+        return checked_codes
 
     @classmethod
     def _validate_options(cls, options):
         """Validate the mutually exclusive options.
 
-        Return `True` iff only zero or one of `MUTUALLY_EXCLUSIVE_OPTIONS` was
-        selected.
+        Return `True` iff only zero or one of `BASE_ERROR_SELECTION_OPTIONS`
+        was selected.
 
         """
         for opt1, opt2 in \
-                itertools.permutations(cls.MUTUALLY_EXCLUSIVE_OPTIONS, 2):
+                itertools.permutations(cls.BASE_ERROR_SELECTION_OPTIONS, 2):
             if getattr(options, opt1) and getattr(options, opt2):
                 log.error('Cannot pass both {0} and {1}. They are '
                           'mutually exclusive.'.format(opt1, opt2))
@@ -1085,27 +1088,34 @@ def _validate_options(cls, options):
     def _has_exclusive_option(cls, options):
         """Return `True` iff one or more exclusive options were selected."""
         return any([getattr(options, opt) for opt in
-                    cls.MUTUALLY_EXCLUSIVE_OPTIONS])
+                    cls.BASE_ERROR_SELECTION_OPTIONS])
 
     @staticmethod
     def _fix_set_options(options):
         """Alter the set options from None/strings to sets in place."""
         optional_set_options = ('ignore', 'select')
         mandatory_set_options = ('add_ignore', 'add_select')
 
+        def _get_set(value):
+            """Split `value` by the delimiter `,` and return a set.
+
+            Removes any occurrences of '' in the set.
+
+            """
+            return set(value.split(',')) - set('')
+
         for opt in optional_set_options:
             value = getattr(options, opt)
             if value is not None:
-                value = set(value.split(','))
-                setattr(options, opt, value)
+                setattr(options, opt, _get_set(value))
 
         for opt in mandatory_set_options:
             value = getattr(options, opt)
             if value is None:
                 value = ''
 
-            if not isinstance(value, set):
-                value = set(value.split(','))
+            if not isinstance(value, Set):
+                value = _get_set(value)
 
             setattr(options, opt, value)
 
@@ -1258,6 +1268,7 @@ def run_pep257():
         for filename, checked_codes in conf.get_files_to_check():
             errors.extend(check((filename,), select=checked_codes))
     except IllegalConfiguration:
+        # An illegal configuration file was found during file generation.
         return INVALID_OPTIONS_RETURN_CODE
 
     code = NO_VIOLATIONS_RETURN_CODE

src/tests/test_pep257.py

@@ -186,6 +186,21 @@ def foo():
         assert 'D103' not in err
 
 
+def test_verbose():
+    """Test that passing --verbose to pep257 prints more information."""
+    with Pep257Env() as env:
+        with env.open('example.py', 'wt') as example:
+            example.write('"""Module docstring."""\n')
+
+        out, _, code = env.invoke_pep257()
+        assert code == 0
+        assert 'example.py' not in out
+
+        out, _, code = env.invoke_pep257(args="--verbose")
+        assert code == 0
+        assert 'example.py' in out
+
+
 def test_count():
     """Test that passing --count to pep257 correctly prints the error num."""
     with Pep257Env() as env:
@@ -405,7 +420,7 @@ def foo():
 def test_config_file_inheritance():
     """Test configuration files inheritance.
 
-    The test creates 3 configuration files:
+    The test creates 2 configuration files:
 
     env_base
     +-- tox.ini
@@ -416,10 +431,13 @@ def test_config_file_inheritance():
         +-- test.py
             The file will contain code that violates D100,D103.
 
-    When invoking pep257 `test.py` the first config file will not set any
-    of select/ignore/convention, therefore `pep257` will be set as the default
-    convention. The config sets `inherit=false`, therefore we expect it not to
-    set `select=` and raise all the errors stated above.
+    When invoking pep257, the first config file found in the base directory
+    will set `select=`, so no error codes should be checked.
+    The `A/tox.ini` configuration file sets `inherit=false` but has an empty
+    configuration, therefore the default convention will be checked.
+
+    We expect pep257 to ignore the `select=` configuration and raise all
+    the errors stated above.
 
     """
     with Pep257Env() as env:
@@ -579,7 +597,7 @@ def test_cli_overrides_config_file():
 
     We shall run pep257 with `--convention=pep257`.
     We expect `base.py` to be checked and violate `D100` and that `A/a.py` will
-    not be checked because of `match-dir=B` in the config file.
+    not be checked because of `match-dir=foo` in the config file.
 
     """
     with Pep257Env() as env: