Bugfix/merge duplicates

src/tctools/patch_plc/patch_plc_class.py

@@ -1,3 +1,4 @@
+import logging
 from argparse import RawDescriptionHelpFormatter
 from collections.abc import Iterable
 from dataclasses import dataclass, field
@@ -180,12 +181,12 @@ def operation_merge(
         """See help info for `merge`."""
         new_sources = FileItems.merge(new_sources.values())
 
-        self.skip_file_duplicates(new_sources, current_sources)
-
         sizes_all = (len(new_sources.folders), len(new_sources.files))
 
         new_sources.subtract(current_sources)
 
+        self.skip_file_duplicates(new_sources, current_sources)
+
         self.logger.info(
             f"Discovered {sizes_all[1]} source files, of which "
             f"{len(new_sources.files)} are unregistered"
@@ -200,6 +201,8 @@ def operation_merge(
             self.logger.info("No new source files or folders, stopping")
             return 0
 
+        self.log_sources(new_sources, True)
+
         if self.args.check:
             self.logger.info("Some file or folders would be added")
             return 1  # Something left to do, so exit with error (= check has failed)
@@ -229,6 +232,8 @@ def operation_remove(
             self.logger.info("No files or folders to un-register, stopping")
             return 0
 
+        self.log_sources(to_remove, False)
+
         if self.args.check:
             self.logger.info("Some file or folders would be un-registered")
             return 1  # Something left to do, so exit with error (= check has failed)
@@ -278,6 +283,9 @@ def operation_reset(self, current_sources: FileItems, new_sources: FileItemsGrou
             self.logger.info("No files or folders to change, stopping")
             return 0
 
+        self.log_sources(to_add, True)
+        self.log_sources(to_remove, False)
+
         if self.args.check:
             self.logger.info("Some file or folders would be (un-)registered")
             return 1  # Something left to do, so exit with error (= check has failed)
@@ -432,6 +440,20 @@ def remove_helper(ref_elements, ref_set):
         remove_helper(self._element_folders, to_remove.folders)
         remove_helper(self._element_files, to_remove.files)
 
+    def log_sources(self, source: FileItems, add: bool):
+        """Log a set of sources in its entirety.
+
+        Logged a INFO level when `dry` or `check` (otherwise the output is kind of
+        meaningless), otherwise at DEBUG.
+        """
+        level = logging.INFO if self.args.check or self.args.dry else logging.DEBUG
+
+        action = "New" if add else "Remove"
+        for item in source.files:
+            self.logger.log(level, f"{action} file: {item}")
+        for item in source.folders:
+            self.logger.log(level, f"{action} folder: {item}")
+
     @staticmethod
     def path_to_str(path: PurePath) -> str:
         """Turn any path into a windows-path string.

tests/test_patch_plc.py

@@ -2,6 +2,7 @@
 import shutil
 import subprocess
 import sys
+from collections.abc import Generator
 from pathlib import Path, PureWindowsPath
 
 import pytest
@@ -21,6 +22,16 @@ def to_paths(*paths: str) -> list[Path]:
     return [Path(p) for p in paths]
 
 
+@pytest.fixture()
+def plc_dir(plc_code) -> Generator[Path, None, None]:
+    yield plc_code / "TwinCAT Project1" / "MyPlc"
+
+
+@pytest.fixture()
+def project(plc_dir) -> Generator[Path, None, None]:
+    yield plc_dir / "MyPlc.plcproj"
+
+
 def test_help(capsys):
     """Test the help text."""
     with pytest.raises(SystemExit) as err:
@@ -32,27 +43,25 @@ def test_help(capsys):
     assert "usage:" in message
 
 
-def test_cli(plc_code):
+def test_cli(plc_dir, project):
     """Test the CLI hook works."""
-    plc_dir = plc_code / "TwinCAT Project1" / "MyPlc"
-    project = plc_dir / "MyPlc.plcproj"
     source = plc_dir / "POUs" / "untracked_source"
 
+    assert "untracked_source" not in project.read_text()
+
     path = sys.executable  # Re-use whatever executable we're using now
     result = subprocess.run(
         [path, "-m", "tctools.patch_plc", str(project), "merge", "-r", str(source)],
         capture_output=True,
     )
 
     assert result.returncode == 0
-    # assert "Re-saved 1 path" in result.stdout.decode()
-    # TODO: Add some kind of output assertion here
+    assert "untracked_source" in project.read_text()
+    # More detailed assertions we will perform in the next tests
 
 
-def test_merge_single_file(plc_code):
+def test_merge_single_file(plc_dir, project):
     """Test happy-flow adding."""
-    plc_dir = plc_code / "TwinCAT Project1" / "MyPlc"
-    project = plc_dir / "MyPlc.plcproj"
     source = plc_dir / "POUs" / "untracked_source" / "F_UntrackedFunc.TcPOU"
 
     patcher = PatchPlc(str(project), "merge", str(source))
@@ -70,11 +79,13 @@ def test_merge_single_file(plc_code):
     assert '<Folder Include="POUs\\untracked_source"/>' in project_content
 
 
-def test_merge_recursive(plc_code):
-    """Test happy-flow adding for a complete folder."""
-    plc_dir = plc_code / "TwinCAT Project1" / "MyPlc"
-    project = plc_dir / "MyPlc.plcproj"
-    source = plc_dir / "POUs" / "untracked_source"
+@pytest.mark.parametrize("target", ["POUs/untracked_source/", "./"])
+def test_merge_recursive(plc_dir, project, target, caplog):
+    """Test happy-flow adding for a complete folder.
+
+    Try both on a completely new folder and the entire project root
+    """
+    source = plc_dir / Path(target)
 
     untracked_files = to_paths(
         "POUs/untracked_source/F_UntrackedFunc.TcPOU",
@@ -92,8 +103,8 @@ def test_merge_recursive(plc_code):
     for path in untracked_files + untracked_folders:
         assert path_to_str(path) not in project_content
 
-    patcher = PatchPlc(str(project), "merge", str(source), "-r")
-    patcher.run()
+    with caplog.at_level(logging.WARNING):
+        PatchPlc(str(project), "merge", str(source), "-r").run()
 
     project_content = project.read_text()
 
@@ -102,19 +113,18 @@ def test_merge_recursive(plc_code):
     for folder in untracked_folders:
         assert f'<Folder Include="{path_to_str(folder)}"/>' in project_content
 
+    assert len(caplog.records) == 0  # No warnings or errors
+
     # Now compare with stored, sorted result:
-    expected_file = plc_code / source / "MyPlc_with_untracked.plcproj.xml"
+    expected_file = plc_dir / "MyPlc_with_untracked.plcproj.xml"
 
     XmlSorter(str(project)).run()
 
     assert project.read_text() == expected_file.read_text()
 
 
-def test_remove(plc_code):
+def test_remove(plc_dir, project):
     """Test remove happy-flow."""
-    plc_dir = plc_code / "TwinCAT Project1" / "MyPlc"
-    project = plc_dir / "MyPlc.plcproj"
-
     tracked_files = to_paths(
         "POUs/FB_Example.TcPOU",
         "DUTs/ST_Example.TcDUT",
@@ -136,14 +146,11 @@ def test_remove(plc_code):
         assert path_to_str(file) not in project_content
 
     lines_after = project_content.count("\n")
-    assert lines_before - 6 == lines_after  # Make sure not more got deleted
+    assert lines_after == lines_before - 6  # Make sure not more got deleted
 
 
 @pytest.mark.parametrize("recursive", [False, True])
-def test_remove_recursive(plc_code, recursive):
-    plc_dir = plc_code / "TwinCAT Project1" / "MyPlc"
-    project = plc_dir / "MyPlc.plcproj"
-
+def test_remove_recursive(plc_dir, project, recursive):
     folder = Path("POUs/Module")
     tracked_folders = [
         'Include="POUs\\Module"',
@@ -173,19 +180,16 @@ def test_remove_recursive(plc_code, recursive):
     lines_after = content_after.count("\n")
 
     if not recursive:
-        assert content_before == content_after  # No changes
+        assert content_after == content_before  # No changes
         return
 
     for item in tracked_files + tracked_folders:
         assert item not in content_after
 
-    assert lines_before - 2 * 1 - 3 * 3 == lines_after
-
+    assert lines_after == lines_before - 2 * 1 - 3 * 3
 
-def test_reset(plc_code):
-    plc_dir = plc_code / "TwinCAT Project1" / "MyPlc"
-    project = plc_dir / "MyPlc.plcproj"
 
+def test_reset(plc_dir, project):
     # Remove the actual (tracked) folder first:
     shutil.rmtree(plc_dir / "POUs" / "Module")
     # And there is already the "POUs/untracked_source" directory
@@ -225,14 +229,11 @@ def test_reset(plc_code):
         assert item in content_after
 
     lines_after = content_after.count("\n")
-    assert lines_before - (2 * 1) - (3 * 3) + (2 * 1) + (4 * 3) == lines_after
+    assert lines_after == lines_before - (2 * 1) - (3 * 3) + (2 * 1) + (4 * 3)
     # Remove 2 folders and 3 files, add 2 folders and 4 files
 
 
-def test_reset_duplicate(plc_code, caplog):
-    plc_dir = plc_code / "TwinCAT Project1" / "MyPlc"
-    project = plc_dir / "MyPlc.plcproj"
-
+def test_reset_duplicate(plc_dir, project, caplog):
     # Create a new file, with a name that's already known:
     module_folder = plc_dir / "POUs" / "Module"
     new_file = module_folder / "DUTs" / "MAIN.TcPOU"
@@ -249,4 +250,4 @@ def test_reset_duplicate(plc_code, caplog):
     msg = caplog.records[0].message
     assert "Refusing to add" in msg and "MAIN.TcPOU" in msg
 
-    assert content_before == project.read_text()  # The project should not be changed
+    assert project.read_text() == content_before  # The project should not be changed