Merge 2f6efd1 into 60d779c

pyproject.toml

@@ -74,7 +74,9 @@ sphinx_rtd_theme = {version = "*", optional = true}
 pydantic = "*"
 autorepr = "*"
 loguru = "*"
-ConfigUpdater = "*"
+# TODO: remove when this is fixed: https://github.com/pyscaffold/configupdater/issues/14
+ConfigUpdater = { git = "https://github.com/pyscaffold/configupdater.git", branch = "multiline-comment" }
+#ConfigUpdater = "*"
 
 [tool.poetry.extras]
 lint = ["pylint"]

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,6 +104,7 @@ extras = doc
 commands =
     sphinx-apidoc --force --module-first --separate --implicit-namespaces --output-dir docs/source src/nitpick/
     python3 docs/generate_rst.py
+    # Options to debug Sphinx: -nWT --keep-going -vvv
     sphinx-build --color -b linkcheck docs "{toxworkdir}/docs_out"
     sphinx-build -d "{toxworkdir}/docs_doctree" --color -b html docs "{toxworkdir}/docs_out" {posargs}
 

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}

tests/test_setup_cfg.py

@@ -1,4 +1,9 @@
 """setup.cfg tests."""
+from configparser import ParsingError
+from unittest import mock
+
+from configupdater import ConfigUpdater
+
 from nitpick.constants import SETUP_CFG
 from nitpick.plugins.setup_cfg import SetupCfgPlugin, Violations
 from nitpick.violations import Fuss, ProjectViolations, SharedViolations
@@ -309,3 +314,109 @@ def test_invalid_sections_comma_separated_values(tmp_path):
     ).api_apply().assert_violations(
         Fuss(False, SETUP_CFG, 325, ": invalid sections on comma_separated_values:", "aaa, falek8")
     )
+
+
+def test_multiline_comment(tmp_path):
+    """Test file with multiline comments should not raise a configparser.ParsingError."""
+    original_file = """
+        [flake8]
+        exclude =
+          # Trash and cache:
+          .git
+          __pycache__
+          .venv
+          .eggs
+          *.egg
+          temp
+          # Bad code that I write to test things:
+          ex.py
+        another =
+            A valid line
+            ; Now a comment with semicolon
+            Another valid line
+        """
+    ProjectMock(tmp_path).style(
+        """
+        ["setup.cfg".flake8]
+        new = "value"
+        """
+    ).setup_cfg(original_file).api_apply().assert_violations(
+        Fuss(
+            True,
+            SETUP_CFG,
+            324,
+            ": section [flake8] has some missing key/value pairs. Use this:",
+            """
+            [flake8]
+            new = value
+            """,
+        )
+    ).assert_file_contents(
+        SETUP_CFG,
+        f"""
+        {original_file}new = value
+        """,
+    )
+
+
+def test_duplicated_option(tmp_path):
+    """Test a violation is raised if a file has a duplicated option."""
+    original_file = """
+        [abc]
+        easy = 123
+        easy = as sunday morning
+        """
+    project = ProjectMock(tmp_path)
+    full_path = project.root_dir / SETUP_CFG
+    project.style(
+        """
+        ["setup.cfg".abc]
+        hard = "as a rock"
+        """
+    ).setup_cfg(original_file).api_apply().assert_violations(
+        Fuss(
+            False,
+            SETUP_CFG,
+            Violations.PARSING_ERROR.code,
+            f": parsing error (DuplicateOptionError): While reading from {str(full_path)!r} "
+            f"[line  3]: option 'easy' in section 'abc' already exists",
+        )
+    ).assert_file_contents(
+        SETUP_CFG, original_file
+    )
+
+
+@mock.patch.object(ConfigUpdater, "update_file")
+def test_simulate_parsing_error_when_saving(update_file, tmp_path):
+    """Simulate a parsing error when saving setup.cfg."""
+    update_file.side_effect = ParsingError(source="simulating a captured error")
+
+    original_file = """
+        [flake8]
+        existing = value
+        """
+    ProjectMock(tmp_path).style(
+        """
+        ["setup.cfg".flake8]
+        new = "value"
+        """
+    ).setup_cfg(original_file).api_apply().assert_violations(
+        Fuss(
+            True,
+            SETUP_CFG,
+            324,
+            ": section [flake8] has some missing key/value pairs. Use this:",
+            """
+            [flake8]
+            new = value
+            """,
+        ),
+        Fuss(
+            False,
+            SETUP_CFG,
+            Violations.PARSING_ERROR.code,
+            ": parsing error (ParsingError): Source contains parsing errors: 'simulating a captured error'",
+        ),
+    ).assert_file_contents(
+        SETUP_CFG, original_file
+    )