fix: display parsing errors as violations (#299)

* test: setup.cfg with multiline comments
* fix: don't abort on parsing error; display a violation instead
* fix: don't change the file if there was a parsing error
* test: simulate a parsing error when saving setup.cfg
* refactor: last_item renamed to last_block in ConfigUpdater
* build: new release of configupdater
* docs: don't fail if a page is unreachable

setup.cfg

@@ -80,8 +80,8 @@ deps =
     pip>=19.2
     safety
 # Run nitpick and pylint with tox, because local repos don't seem to work well with https://pre-commit.ci/
-# Run Nitpick locally on itself
 commands =
+    # Run Nitpick locally on itself
     flake8 --select=NIP
     pylint src/
     safety check
@@ -104,8 +104,11 @@ extras = doc
 commands =
     sphinx-apidoc --force --module-first --separate --implicit-namespaces --output-dir docs/source src/nitpick/
     python3 docs/generate_rst.py
-    sphinx-build --color -b linkcheck docs "{toxworkdir}/docs_out"
-    sphinx-build -d "{toxworkdir}/docs_doctree" --color -b html docs "{toxworkdir}/docs_out" {posargs}
+    # Don't fail if a page is unreachable.
+    # https://tox.readthedocs.io/en/latest/example/basic.html#ignoring-exit-code
+    - sphinx-build --color -j auto -b linkcheck docs "{toxworkdir}/docs_out"
+    # Use these options to debug Sphinx: -nWT --keep-going -vvv
+    sphinx-build --color -j auto -d "{toxworkdir}/docs_doctree" -b html docs "{toxworkdir}/docs_out" {posargs}
 
 [coverage:run]
 # https://coverage.readthedocs.io/en/latest/config.html#run

src/nitpick/cli.py

@@ -10,7 +10,7 @@
 - When you import __main__ it will get executed again (as a module) because
     there's no ``nitpick.__main__`` in ``sys.modules``.
 
-Also see (1) from http://click.pocoo.org/5/setuptools/#setuptools-integration
+Also see (1) from https://click.palletsprojects.com/en/5.x/setuptools/#setuptools-integration
 """
 from pathlib import Path
 from typing import Optional

src/nitpick/plugins/base.py

@@ -68,24 +68,31 @@ def entry_point(self) -> Iterator[Fuss]:
         """Entry point of the Nitpick plugin."""
         self.init()
 
-        has_config_dict = bool(self.expected_config or self.nitpick_file_dict)
         should_exist: bool = bool(self.info.project.nitpick_files_section.get(self.filename, True))
-        file_exists = self.file_path.exists()
-        should_write = True
-
-        if has_config_dict and not file_exists:
-            yield from self._suggest_when_file_not_found()
-        elif not should_exist and file_exists:
+        if self.file_path.exists() and not should_exist:
             logger.info(f"{self}: File {self.filename} exists when it should not")
             # Only display this message if the style is valid.
             yield self.reporter.make_fuss(SharedViolations.DELETE_FILE)
-            should_write = False
-        elif file_exists and has_config_dict:
+            return
+
+        has_config_dict = bool(self.expected_config or self.nitpick_file_dict)
+        if not has_config_dict:
+            return
+
+        yield from self._enforce_file_configuration()
+
+    def _enforce_file_configuration(self):
+        file_exists = self.file_path.exists()
+        if file_exists:
             logger.info(f"{self}: Enforcing rules")
             yield from self.enforce_rules()
+        else:
+            yield from self._suggest_when_file_not_found()
 
-        if should_write and self.apply:
-            self.write_file(file_exists)
+        if self.apply:
+            fuss = self.write_file(file_exists)  # pylint: disable=assignment-from-none
+            if fuss:
+                yield fuss
 
     def init(self):
         """Hook for plugin initialization after the instance was created."""
@@ -101,8 +108,9 @@ def _suggest_when_file_not_found(self):
         else:
             yield self.reporter.make_fuss(SharedViolations.CREATE_FILE)
 
-    def write_file(self, file_exists: bool) -> None:
+    def write_file(self, file_exists: bool) -> Optional[Fuss]:  # pylint: disable=unused-argument,no-self-use
         """Hook to write the new file when apply mode is on. Should be used by inherited classes."""
+        return None
 
     @abc.abstractmethod
     def enforce_rules(self) -> Iterator[Fuss]:

src/nitpick/plugins/setup_cfg.py

@@ -1,5 +1,5 @@
 """Enforce config on `setup.cfg <https://docs.python.org/3/distutils/configfile.html>`."""
-from configparser import ConfigParser
+from configparser import ConfigParser, DuplicateOptionError, ParsingError
 from io import StringIO
 from typing import Any, Dict, Iterator, List, Optional, Set, Tuple, Type
 
@@ -24,6 +24,7 @@ class Violations(ViolationEnum):
     KEY_HAS_DIFFERENT_VALUE = (323, ": [{section}]{key} is {actual} but it should be like this:")
     MISSING_KEY_VALUE_PAIRS = (324, ": section [{section}] has some missing key/value pairs. Use this:")
     INVALID_COMMA_SEPARATED_VALUES_SECTION = (325, f": invalid sections on {COMMA_SEPARATED_VALUES}:")
+    PARSING_ERROR = (326, ": parsing error ({cls}): {msg}")
 
 
 class SetupCfgPlugin(NitpickPlugin):
@@ -64,12 +65,16 @@ def missing_sections(self) -> Set[str]:
         """Missing sections."""
         return self.expected_sections - self.current_sections
 
-    def write_file(self, file_exists: bool) -> None:
+    def write_file(self, file_exists: bool) -> Optional[Fuss]:
         """Write the new file."""
-        if file_exists:
-            self.updater.update_file()
-        else:
-            self.updater.write(self.file_path.open("w"))
+        try:
+            if file_exists:
+                self.updater.update_file()
+            else:
+                self.updater.write(self.file_path.open("w"))
+        except ParsingError as err:
+            return self.reporter.make_fuss(Violations.PARSING_ERROR, cls=err.__class__.__name__, msg=err)
+        return None
 
     def get_missing_output(self) -> str:
         """Get a missing output string example from the missing sections in setup.cfg."""
@@ -81,8 +86,8 @@ def get_missing_output(self) -> str:
         for section in sorted(missing):
             expected_config: Dict = self.expected_config[section]
             if self.apply:
-                if self.updater.last_item:
-                    self.updater.last_item.add_after.space(1)
+                if self.updater.last_block:
+                    self.updater.last_block.add_after.space(1)
                 self.updater.add_section(section)
                 self.updater[section].update(expected_config)
             parser[section] = expected_config
@@ -91,7 +96,14 @@ def get_missing_output(self) -> str:
     # TODO: convert the contents to dict (with IniConfig().sections?) and mimic other plugins doing dict diffs
     def enforce_rules(self) -> Iterator[Fuss]:
         """Enforce rules on missing sections and missing key/value pairs in setup.cfg."""
-        self.updater.read(str(self.file_path))
+        try:
+            self.updater.read(str(self.file_path))
+        except DuplicateOptionError as err:
+            # Don't change the file if there was a parsing error
+            self.apply = False
+            yield self.reporter.make_fuss(Violations.PARSING_ERROR, cls=err.__class__.__name__, msg=err)
+            return
+
         yield from self.enforce_missing_sections()
 
         csv_sections = {v.split(".")[0] for v in self.comma_separated_values}

tasks.py

@@ -80,7 +80,7 @@ def doc(c):
     c.run("tox -e docs")
 
 
-@task
+@task(help={"full": "Full build using tox", "recreate": "Recreate tox environment"})
 def ci_build(c, full=False, recreate=False):
     """Simulate a CI build."""
     tox_cmd = "tox -r" if recreate else "tox"