Add tests for tach mod edits, fix issues w pre-emptive validation and…

… missing location on fresh config

python/tach/extension.pyi

@@ -155,6 +155,7 @@ class ProjectConfig:
 
     def __new__(cls) -> ProjectConfig: ...
     def serialize_json(self) -> str: ...
+    def set_location(self, location: Path) -> None: ...
     def has_no_modules(self) -> bool: ...
     def has_no_dependencies(self) -> bool: ...
     def module_paths(self) -> list[str]: ...

python/tach/mod.py

@@ -91,6 +91,7 @@ def apply_selected_configuration(
     if not project_config_path.exists():
         config_toml_content = dump_project_config_to_toml(project_config)
         project_config_path.write_text(config_toml_content)
+        project_config.set_location(project_config_path)
 
     relative_selected_source_roots = [
         str(source_root.relative_to(project_root))

python/tests/test_mod.py

@@ -0,0 +1,157 @@
+from __future__ import annotations
+
+from unittest.mock import patch
+
+import pytest
+
+from tach.extension import ProjectConfig
+from tach.interactive import InteractiveModuleConfiguration
+from tach.mod import mod_edit_interactive
+from tach.parsing import parse_project_config
+
+
+@pytest.fixture
+def temp_project_dir(tmp_path):
+    """Create a temporary project directory with initial structure."""
+    project_root = tmp_path / "test_project"
+    project_root.mkdir()
+    return project_root
+
+
+@pytest.fixture
+def initial_project_config():
+    """Create a clean project config."""
+    return ProjectConfig()
+
+
+def test_mod_edit_interactive_new_configuration(
+    temp_project_dir, initial_project_config
+):
+    mock_config = InteractiveModuleConfiguration(
+        source_roots=[temp_project_dir / "src"],
+        module_paths=[
+            temp_project_dir / "src" / "module1.py",
+            temp_project_dir / "src" / "module2.py",
+        ],
+        utility_paths=[temp_project_dir / "src" / "utils.py"],
+    )
+
+    with patch(
+        "tach.mod.get_selected_modules_interactive", return_value=mock_config
+    ) as mock_get:
+        success, errors = mod_edit_interactive(
+            project_root=temp_project_dir,
+            project_config=initial_project_config,
+            exclude_paths=[],
+        )
+
+        assert success is True
+        assert not errors
+        mock_get.assert_called_once()
+
+        # Verify the saved configuration
+        saved_config = parse_project_config(temp_project_dir)
+        assert set(saved_config.source_roots) == {"src"}
+        assert set(saved_config.module_paths()) == {"module1", "module2", "utils"}
+        assert set(saved_config.utility_paths()) == {"utils"}
+
+
+def test_mod_edit_interactive_validation_error(
+    temp_project_dir, initial_project_config
+):
+    # Create a configuration where a module is outside the source roots
+    mock_config = InteractiveModuleConfiguration(
+        source_roots=[temp_project_dir / "src"],
+        module_paths=[temp_project_dir / "outside" / "module1.py"],  # Outside src
+        utility_paths=[],
+    )
+
+    with patch(
+        "tach.mod.get_selected_modules_interactive", return_value=mock_config
+    ) as mock_get:
+        success, errors = mod_edit_interactive(
+            project_root=temp_project_dir,
+            project_config=initial_project_config,
+            exclude_paths=[],
+        )
+
+        assert success is False
+        assert len(errors) == 1
+        assert "not contained within any source root" in errors[0]
+        mock_get.assert_called_once()
+
+
+def test_mod_edit_interactive_user_cancelled(temp_project_dir, initial_project_config):
+    with patch(
+        "tach.mod.get_selected_modules_interactive", return_value=None
+    ) as mock_get:
+        success, errors = mod_edit_interactive(
+            project_root=temp_project_dir,
+            project_config=initial_project_config,
+            exclude_paths=[],
+        )
+
+        assert success is False
+        assert len(errors) == 1
+        assert "No changes saved" in errors[0]
+        mock_get.assert_called_once()
+
+
+def test_mod_edit_interactive_update_existing(temp_project_dir, initial_project_config):
+    # First round of edits
+    mock_config = InteractiveModuleConfiguration(
+        source_roots=[temp_project_dir / "src", temp_project_dir / "tests"],
+        module_paths=[
+            temp_project_dir / "src" / "new_module.py",
+        ],
+        utility_paths=[
+            temp_project_dir / "src" / "new_utility.py",
+        ],
+    )
+
+    with patch(
+        "tach.mod.get_selected_modules_interactive", return_value=mock_config
+    ) as mock_get:
+        success, errors = mod_edit_interactive(
+            project_root=temp_project_dir,
+            project_config=initial_project_config,
+            exclude_paths=[],
+        )
+
+        assert success is True
+        assert not errors
+        mock_get.assert_called_once()
+
+        saved_config = parse_project_config(temp_project_dir)
+        assert set(saved_config.source_roots) == {"src", "tests"}
+        assert set(saved_config.module_paths()) == {"new_module", "new_utility"}
+        assert set(saved_config.utility_paths()) == {"new_utility"}
+
+    # Second round of edits
+    mock_config_2 = InteractiveModuleConfiguration(
+        source_roots=[temp_project_dir / "src"],  # Remove tests directory
+        module_paths=[
+            temp_project_dir / "src" / "new_module.py",
+            temp_project_dir / "src" / "another_module.py",  # Add new module
+        ],
+        utility_paths=[],  # Remove utility
+    )
+
+    with patch(
+        "tach.mod.get_selected_modules_interactive", return_value=mock_config_2
+    ) as mock_get:
+        success, errors = mod_edit_interactive(
+            project_root=temp_project_dir,
+            project_config=saved_config,  # Use previously saved config
+            exclude_paths=[],
+        )
+
+        assert success is True
+        assert not errors
+        mock_get.assert_called_once()
+
+        # Verify final configuration
+        final_config = parse_project_config(temp_project_dir)
+        assert set(final_config.source_roots) == {"src"}
+        assert set(final_config.module_paths()) == {"new_module", "another_module"}
+        assert set(final_config.utility_paths()) == set()

src/config/domain.rs

@@ -225,32 +225,16 @@ impl LocatedDomainConfig {
 impl ConfigEditor for LocatedDomainConfig {
     fn enqueue_edit(&mut self, edit: &ConfigEdit) -> Result<(), EditError> {
         match edit {
-            ConfigEdit::CreateModule { path } => {
-                if self.modules().any(|module| module.path == *path) {
-                    Err(EditError::ModuleAlreadyExists)
-                } else if path.starts_with(&self.location.mod_path) {
-                    // Loose check, if the module path starts with the domain path,
-                    // then it belongs to this domain
-                    self.pending_edits.push(edit.clone());
-                    Ok(())
-                } else {
-                    Err(EditError::NotApplicable)
-                }
-            }
-            ConfigEdit::DeleteModule { path }
+            ConfigEdit::CreateModule { path }
+            | ConfigEdit::DeleteModule { path }
             | ConfigEdit::MarkModuleAsUtility { path }
             | ConfigEdit::UnmarkModuleAsUtility { path }
             | ConfigEdit::AddDependency { path, .. }
             | ConfigEdit::RemoveDependency { path, .. } => {
                 if path.starts_with(&self.location.mod_path) {
-                    // If this module path appears to belong to this domain,
-                    // and the module exists, enqueue the edit
-                    if self.modules().any(|module| module.path == *path) {
-                        self.pending_edits.push(edit.clone());
-                        Ok(())
-                    } else {
-                        Err(EditError::ModuleNotFound)
-                    }
+                    // If this module path appears to belong to this domain, enqueue the edit
+                    self.pending_edits.push(edit.clone());
+                    Ok(())
                 } else {
                     Err(EditError::NotApplicable)
                 }

src/config/project.rs

@@ -131,10 +131,6 @@ impl ProjectConfig {
             .map(|mod_config| mod_config.depends_on.as_ref())?
     }
 
-    pub fn set_location(&mut self, location: PathBuf) {
-        self.location = Some(location);
-    }
-
     pub fn absolute_source_roots(&self) -> Result<Vec<PathBuf>, ConfigError> {
         let project_root = self
             .location
@@ -212,31 +208,18 @@ impl ConfigEditor for ProjectConfig {
             .collect::<Vec<Result<(), EditError>>>();
 
         let result = match edit {
-            ConfigEdit::CreateModule { path } => {
+            ConfigEdit::CreateModule { .. }
+            | ConfigEdit::DeleteModule { .. }
+            | ConfigEdit::MarkModuleAsUtility { .. }
+            | ConfigEdit::UnmarkModuleAsUtility { .. }
+            | ConfigEdit::AddDependency { .. }
+            | ConfigEdit::RemoveDependency { .. } => {
                 if !domain_results.iter().any(|r| r.is_ok()) {
-                    if self.modules.iter().any(|module| module.path == *path) {
-                        Err(EditError::ModuleAlreadyExists)
-                    } else {
-                        // If no domain will create the module, and the module doesn't already exist,
-                        // enqueue the edit
-                        self.pending_edits.push(edit.clone());
-                        Ok(())
-                    }
-                } else {
-                    Err(EditError::NotApplicable)
-                }
-            }
-            ConfigEdit::DeleteModule { path }
-            | ConfigEdit::MarkModuleAsUtility { path }
-            | ConfigEdit::UnmarkModuleAsUtility { path }
-            | ConfigEdit::AddDependency { path, .. }
-            | ConfigEdit::RemoveDependency { path, .. } => {
-                // If we know of this module, enqueue the edit
-                if self.modules.iter().any(|module| module.path == *path) {
+                    // If no domain accepted the edit, enqueue the edit
                     self.pending_edits.push(edit.clone());
                     Ok(())
                 } else {
-                    Err(EditError::ModuleNotFound)
+                    Err(EditError::NotApplicable)
                 }
             }
             ConfigEdit::AddSourceRoot { .. } | ConfigEdit::RemoveSourceRoot { .. } => {
@@ -459,6 +442,10 @@ impl ProjectConfig {
         serde_json::to_string(&self).unwrap()
     }
 
+    pub fn set_location(&mut self, location: PathBuf) {
+        self.location = Some(location);
+    }
+
     pub fn has_no_modules(&self) -> bool {
         self.all_modules().next().is_none()
     }