From fca496fc080d50cea7c1c24755199227cef027fb Mon Sep 17 00:00:00 2001 From: David Seddon Date: Fri, 5 Sep 2025 16:53:19 +0100 Subject: [PATCH 01/16] Implement write method for BasicFileSystem --- rust/src/filesystem.rs | 25 +++++++++++++++++++++++ src/grimp/application/ports/filesystem.py | 4 +++- tests/unit/adaptors/test_filesystem.py | 8 ++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/rust/src/filesystem.rs b/rust/src/filesystem.rs index 4b54781c..5eb671eb 100644 --- a/rust/src/filesystem.rs +++ b/rust/src/filesystem.rs @@ -5,6 +5,8 @@ use regex::Regex; use std::collections::HashMap; use std::ffi::OsStr; use std::fs; +use std::fs::File; +use std::io::prelude::*; use std::path::{Path, PathBuf}; use std::sync::LazyLock; use unindent::unindent; @@ -22,6 +24,8 @@ pub trait FileSystem: Send + Sync { fn exists(&self, file_name: &str) -> bool; fn read(&self, file_name: &str) -> PyResult; + + fn write(&mut self, file_name: &str, contents: &str) -> PyResult<()>; } #[derive(Clone)] @@ -129,6 +133,12 @@ impl FileSystem for RealBasicFileSystem { }) } } + + fn write(&mut self, file_name: &str, contents: &str) -> PyResult<()> { + let mut file = File::create(file_name)?; + file.write_all(contents.as_bytes())?; + Ok(()) + } } #[pymethods] @@ -161,6 +171,10 @@ impl PyRealBasicFileSystem { fn read(&self, file_name: &str) -> PyResult { self.inner.read(file_name) } + + fn write(&mut self, file_name: &str, contents: &str) -> PyResult<()> { + self.inner.write(file_name, contents) + } } type FileSystemContents = HashMap; @@ -243,6 +257,13 @@ impl FileSystem for FakeBasicFileSystem { ))), } } + + #[allow(unused_variables)] + fn write(&mut self, file_name: &str, contents: &str) -> PyResult<()> { + self.contents + .insert(file_name.to_string(), contents.to_string()); + Ok(()) + } } #[pymethods] @@ -278,6 +299,10 @@ impl PyFakeBasicFileSystem { self.inner.read(file_name) } + fn write(&mut self, file_name: &str, contents: &str) -> PyResult<()> { + self.inner.write(file_name, contents) + } + // Temporary workaround method for Python tests. fn convert_to_basic(&self) -> PyResult { Ok(PyFakeBasicFileSystem { diff --git a/src/grimp/application/ports/filesystem.py b/src/grimp/application/ports/filesystem.py index 6597f8ac..268b1efe 100644 --- a/src/grimp/application/ports/filesystem.py +++ b/src/grimp/application/ports/filesystem.py @@ -93,7 +93,7 @@ def convert_to_basic(self) -> BasicFileSystem: class BasicFileSystem(Protocol): """ - A more limited file system, used by the Rust-based scan_for_imports function. + A more limited file system. Having two different file system APIs is an interim approach, allowing us to implement BasicFileSystem in Rust without needing to implement the full range @@ -109,4 +109,6 @@ def split(self, file_name: str) -> tuple[str, str]: ... def read(self, file_name: str) -> str: ... + def write(self, file_name: str, contents: str) -> None: ... + def exists(self, file_name: str) -> bool: ... diff --git a/tests/unit/adaptors/test_filesystem.py b/tests/unit/adaptors/test_filesystem.py index 6644b7bc..e769902b 100644 --- a/tests/unit/adaptors/test_filesystem.py +++ b/tests/unit/adaptors/test_filesystem.py @@ -147,6 +147,14 @@ def test_read(self, file_name, expected_contents): else: assert file_system.read(file_name) == expected_contents + def test_write(self): + some_filename, some_contents = "path/to/some-file.txt", "Some contents." + file_system = self.file_system_cls() + + file_system.write(some_filename, some_contents) + + assert file_system.read(some_filename) == some_contents + class TestFakeFileSystem(_Base): file_system_cls = FakeFileSystem From 5ef803b63e466e384fb08d2a1eae0f735dc41a25 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Fri, 5 Sep 2025 17:04:10 +0100 Subject: [PATCH 02/16] Use BasicFileSystem in the cache Because this is defined in rust, it allows us to push down more logic into Rust. --- src/grimp/adaptors/caching.py | 6 +++--- src/grimp/application/ports/caching.py | 6 +++--- src/grimp/application/usecases.py | 2 +- tests/unit/adaptors/test_caching.py | 18 +++++++++--------- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/grimp/adaptors/caching.py b/src/grimp/adaptors/caching.py index 0782923c..df29916f 100644 --- a/src/grimp/adaptors/caching.py +++ b/src/grimp/adaptors/caching.py @@ -4,7 +4,7 @@ import logging from typing import Optional -from grimp.application.ports.filesystem import AbstractFileSystem +from grimp.application.ports.filesystem import BasicFileSystem from grimp.application.ports.modulefinder import FoundPackage, ModuleFile from grimp.domain.valueobjects import DirectImport, Module @@ -77,7 +77,7 @@ def __init__(self, *args, namer: type[CacheFileNamer], **kwargs) -> None: @classmethod def setup( cls, - file_system: AbstractFileSystem, + file_system: BasicFileSystem, found_packages: set[FoundPackage], include_external_packages: bool, exclude_type_checking_imports: bool = False, @@ -202,7 +202,7 @@ def _read_data_map_file(self) -> dict[Module, set[DirectImport]]: ) try: imports_by_module = rust.read_cache_data_map_file( - data_cache_filename, self.file_system.convert_to_basic() + data_cache_filename, self.file_system ) except FileNotFoundError: logger.info(f"No cache file: {data_cache_filename}.") diff --git a/src/grimp/application/ports/caching.py b/src/grimp/application/ports/caching.py index e04a1362..c271ddd4 100644 --- a/src/grimp/application/ports/caching.py +++ b/src/grimp/application/ports/caching.py @@ -1,7 +1,7 @@ from grimp.application.ports.modulefinder import FoundPackage, ModuleFile from grimp.domain.valueobjects import DirectImport, Module -from .filesystem import AbstractFileSystem +from .filesystem import BasicFileSystem class CacheMiss(Exception): @@ -11,7 +11,7 @@ class CacheMiss(Exception): class Cache: def __init__( self, - file_system: AbstractFileSystem, + file_system: BasicFileSystem, include_external_packages: bool, exclude_type_checking_imports: bool, found_packages: set[FoundPackage], @@ -29,7 +29,7 @@ def __init__( @classmethod def setup( cls, - file_system: AbstractFileSystem, + file_system: BasicFileSystem, found_packages: set[FoundPackage], *, include_external_packages: bool, diff --git a/src/grimp/application/usecases.py b/src/grimp/application/usecases.py index ef5b858a..fb228770 100644 --- a/src/grimp/application/usecases.py +++ b/src/grimp/application/usecases.py @@ -110,7 +110,7 @@ def _scan_packages( if cache_dir is not None: cache_dir_if_supplied = cache_dir if cache_dir != NotSupplied else None cache: caching.Cache = settings.CACHE_CLASS.setup( - file_system=file_system, + file_system=file_system.convert_to_basic(), found_packages=found_packages, include_external_packages=include_external_packages, exclude_type_checking_imports=exclude_type_checking_imports, diff --git a/tests/unit/adaptors/test_caching.py b/tests/unit/adaptors/test_caching.py index 77673520..ea57d372 100644 --- a/tests/unit/adaptors/test_caching.py +++ b/tests/unit/adaptors/test_caching.py @@ -202,7 +202,7 @@ class TestCache: ] }""", }, - ) + ).convert_to_basic() MODULE_FILE_UNMODIFIED = ModuleFile( module=Module("mypackage.foo.unmodified"), mtime=SOME_MTIME ) @@ -249,7 +249,7 @@ def test_logs_missing_cache_files(self, caplog): caplog.set_level(logging.INFO, logger=Cache.__module__) Cache.setup( - file_system=FakeFileSystem(), # No cache files. + file_system=FakeFileSystem().convert_to_basic(), # No cache files. found_packages=self.FOUND_PACKAGES, namer=SimplisticFileNamer, include_external_packages=False, @@ -277,7 +277,7 @@ def test_logs_corrupt_cache_meta_file_reading(self, serialized_mtime: str, caplo }}""", ".grimp_cache/mypackage.data.json": "{}", }, - ) + ).convert_to_basic() Cache.setup( file_system=file_system, found_packages=self.FOUND_PACKAGES, @@ -305,7 +305,7 @@ def test_logs_corrupt_cache_data_file_reading(self, caplog): }}""", ".grimp_cache/mypackage.data.json": "INVALID JSON", }, - ) + ).convert_to_basic() Cache.setup( file_system=file_system, @@ -407,7 +407,7 @@ def test_raises_cache_miss_for_missing_module_from_data(self): }}""", ".grimp_cache/mypackage.data.json": """{}""", }, - ) + ).convert_to_basic() module_file = ModuleFile(module=Module("mypackage.somemodule"), mtime=self.SOME_MTIME) cache = Cache.setup( file_system=file_system, @@ -450,7 +450,7 @@ def test_raises_cache_miss_for_corrupt_meta_file(self, serialized_mtime): ] }""", }, - ) + ).convert_to_basic() cache = Cache.setup( file_system=file_system, found_packages=self.FOUND_PACKAGES, @@ -486,7 +486,7 @@ def test_raises_cache_miss_for_corrupt_data_file(self, serialized_import): ] }}""", }, - ) + ).convert_to_basic() cache = Cache.setup( file_system=file_system, found_packages=self.FOUND_PACKAGES, @@ -565,7 +565,7 @@ def test_write_to_cache( self, include_external_packages, expected_data_file_name, cache_dir, caplog ): caplog.set_level(logging.INFO, logger=Cache.__module__) - file_system = FakeFileSystem() + file_system = FakeFileSystem().convert_to_basic() blue_one = Module(name="blue.one") blue_two = Module(name="blue.two") green_one = Module(name="green.one") @@ -672,7 +672,7 @@ def test_write_to_cache( def test_write_to_cache_adds_marker_files(self): some_cache_dir = "/tmp/some-cache-dir" - file_system = FakeFileSystem() + file_system = FakeFileSystem().convert_to_basic() cache = Cache.setup( file_system=file_system, cache_dir=some_cache_dir, From 9d515ece056a93261b710fa2ab81bef35b9a2347 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Fri, 5 Sep 2025 17:59:19 +0100 Subject: [PATCH 03/16] WIP Trying to get writing to work Tests are failing because the file system isn't stateful between calls across the boundary (I think because we clone the inner file system when passing it in). Is this inevitable? --- rust/src/caching.rs | 39 +++++++++++++++++++++++++++++ rust/src/filesystem.rs | 7 +++++- rust/src/lib.rs | 3 +++ src/grimp/adaptors/caching.py | 28 +++++++++++++-------- tests/unit/adaptors/test_caching.py | 11 ++++++-- 5 files changed, 75 insertions(+), 13 deletions(-) diff --git a/rust/src/caching.rs b/rust/src/caching.rs index cb00b417..6ae92714 100644 --- a/rust/src/caching.rs +++ b/rust/src/caching.rs @@ -6,6 +6,30 @@ use pyo3::types::PyDict; use pyo3::{Bound, PyAny, PyResult, Python, pyfunction}; use std::collections::{HashMap, HashSet}; +/// Writes the cache file containing all the imports for a given package. +/// Args: +/// - filename: str +/// - imports_by_module: dict[Module, Set[DirectImport]] +/// - file_system: The file system interface to use. (A BasicFileSystem.) +#[pyfunction] +pub fn write_cache_data_map_file<'py>( + py: Python<'py>, + filename: &str, + imports_by_module: Bound<'py, PyDict>, + file_system: Bound<'py, PyAny>, +) -> PyResult<()> { + eprintln!("About to clone for write."); + let mut file_system_boxed = get_file_system_boxed(&file_system)?; + + let imports_by_module_rust = imports_by_module_to_rust(imports_by_module); + + let file_contents = serialize_imports_by_module(&imports_by_module_rust); + + file_system_boxed.write(filename, &format!("{} HELLO", &file_contents))?; + + Ok(()) +} + /// Reads the cache file containing all the imports for a given package. /// Args: /// - filename: str @@ -17,6 +41,7 @@ pub fn read_cache_data_map_file<'py>( filename: &str, file_system: Bound<'py, PyAny>, ) -> PyResult> { + eprintln!("About to clone for read."); let file_system_boxed = get_file_system_boxed(&file_system)?; let file_contents = file_system_boxed.read(filename)?; @@ -26,6 +51,20 @@ pub fn read_cache_data_map_file<'py>( Ok(imports_by_module_to_py(py, imports_by_module)) } +#[allow(unused_variables)] +fn imports_by_module_to_rust( + imports_by_module: Bound, +) -> HashMap> { + HashMap::new() +} + +#[allow(unused_variables)] +fn serialize_imports_by_module( + imports_by_module: &HashMap>, +) -> String { + "".to_string() +} + pub fn parse_json_to_map( json_str: &str, filename: &str, diff --git a/rust/src/filesystem.rs b/rust/src/filesystem.rs index 5eb671eb..a927e442 100644 --- a/rust/src/filesystem.rs +++ b/rust/src/filesystem.rs @@ -250,6 +250,7 @@ impl FileSystem for FakeBasicFileSystem { } fn read(&self, file_name: &str) -> PyResult { + eprintln!("{:?}", &self.contents.keys()); match self.contents.get(file_name) { Some(file_name) => Ok(file_name.clone()), None => Err(PyFileNotFoundError::new_err(format!( @@ -260,8 +261,11 @@ impl FileSystem for FakeBasicFileSystem { #[allow(unused_variables)] fn write(&mut self, file_name: &str, contents: &str) -> PyResult<()> { + eprintln!("Writing into fake {}, {}", file_name, contents); + eprintln!("Contents currently {:?}", self.contents); self.contents .insert(file_name.to_string(), contents.to_string()); + eprintln!("Contents now {:?}", self.contents); Ok(()) } } @@ -406,7 +410,7 @@ pub fn get_file_system_boxed<'py>( file_system: &Bound<'py, PyAny>, ) -> PyResult> { let file_system_boxed: Box; - + eprintln!("Cloning file system."); if let Ok(py_real) = file_system.extract::>() { file_system_boxed = Box::new(py_real.inner.clone()); } else if let Ok(py_fake) = file_system.extract::>() { @@ -416,5 +420,6 @@ pub fn get_file_system_boxed<'py>( "file_system must be an instance of RealBasicFileSystem or FakeBasicFileSystem", )); } + Ok(file_system_boxed) } diff --git a/rust/src/lib.rs b/rust/src/lib.rs index e0453f26..e63b501b 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -18,6 +18,9 @@ mod _rustgrimp { #[pymodule_export] use crate::caching::read_cache_data_map_file; + #[pymodule_export] + use crate::caching::write_cache_data_map_file; + #[pymodule_export] use crate::graph::GraphWrapper; diff --git a/src/grimp/adaptors/caching.py b/src/grimp/adaptors/caching.py index df29916f..dbfcfa4b 100644 --- a/src/grimp/adaptors/caching.py +++ b/src/grimp/adaptors/caching.py @@ -120,8 +120,23 @@ def write( self, imports_by_module: dict[Module, set[DirectImport]], ) -> None: - self._write_marker_files_if_not_already_there() # Write data file. + data_cache_filename = self.file_system.join( + self.cache_dir, + self._namer.make_data_file_name( + found_packages=self.found_packages, + include_external_packages=self.include_external_packages, + exclude_type_checking_imports=self.exclude_type_checking_imports, + ), + ) + # Rust version + rust.write_cache_data_map_file( + filename=data_cache_filename, + imports_by_module=imports_by_module, + file_system=self.file_system, + ) + self._write_marker_files_if_not_already_there() + # Python version primitives_map: PrimitiveFormat = {} for found_package in self.found_packages: primitives_map_for_found_package: PrimitiveFormat = { @@ -138,15 +153,8 @@ def write( primitives_map.update(primitives_map_for_found_package) serialized = json.dumps(primitives_map) - data_cache_filename = self.file_system.join( - self.cache_dir, - self._namer.make_data_file_name( - found_packages=self.found_packages, - include_external_packages=self.include_external_packages, - exclude_type_checking_imports=self.exclude_type_checking_imports, - ), - ) - self.file_system.write(data_cache_filename, serialized) + # self.file_system.write(data_cache_filename, serialized) + logger.info(f"Wrote data cache file {data_cache_filename}.") # Write meta files. diff --git a/tests/unit/adaptors/test_caching.py b/tests/unit/adaptors/test_caching.py index ea57d372..96454f3f 100644 --- a/tests/unit/adaptors/test_caching.py +++ b/tests/unit/adaptors/test_caching.py @@ -553,12 +553,19 @@ def test_uses_cache_multiple_packages( | expected_additional_imports ) - @pytest.mark.parametrize("cache_dir", ("/tmp/some-cache-dir", "/tmp/some-cache-dir/", None)) + @pytest.mark.parametrize( + "cache_dir", + ( + "/tmp/some-cache-dir", + # "/tmp/some-cache-dir/", + # None, + ), + ) @pytest.mark.parametrize( "include_external_packages, expected_data_file_name", ( (False, "blue,green.data.json"), - (True, "blue,green:external.data.json"), + # (True, "blue,green:external.data.json"), ), ) def test_write_to_cache( From f1fc71b9399c0db177dd184414420524f9f29d6d Mon Sep 17 00:00:00 2001 From: David Seddon Date: Fri, 5 Sep 2025 18:39:43 +0100 Subject: [PATCH 04/16] WIP Write json with rust There are a couple of problems with this: - It's incompatible with the old json format for some reason. - The unit tests don't work because the file system that's passed in doesn't maintain its state between the two read / write calls. --- rust/src/caching.rs | 47 +++++++++++++++++++++++++++++++++---- rust/src/import_scanning.rs | 16 +++++++++++++ rust/src/module_finding.rs | 1 + 3 files changed, 59 insertions(+), 5 deletions(-) diff --git a/rust/src/caching.rs b/rust/src/caching.rs index 6ae92714..e2abf021 100644 --- a/rust/src/caching.rs +++ b/rust/src/caching.rs @@ -2,9 +2,12 @@ use crate::errors::{GrimpError, GrimpResult}; use crate::filesystem::get_file_system_boxed; use crate::import_scanning::{DirectImport, imports_by_module_to_py}; use crate::module_finding::Module; -use pyo3::types::PyDict; +use pyo3::types::PyAnyMethods; +use pyo3::types::{PyDict, PySet}; +use pyo3::types::{PyDictMethods, PySetMethods}; use pyo3::{Bound, PyAny, PyResult, Python, pyfunction}; use std::collections::{HashMap, HashSet}; +use serde_json; /// Writes the cache file containing all the imports for a given package. /// Args: @@ -25,7 +28,7 @@ pub fn write_cache_data_map_file<'py>( let file_contents = serialize_imports_by_module(&imports_by_module_rust); - file_system_boxed.write(filename, &format!("{} HELLO", &file_contents))?; + file_system_boxed.write(filename, &file_contents)?; Ok(()) } @@ -53,16 +56,50 @@ pub fn read_cache_data_map_file<'py>( #[allow(unused_variables)] fn imports_by_module_to_rust( - imports_by_module: Bound, + imports_by_module_py: Bound, ) -> HashMap> { - HashMap::new() + let mut imports_by_module_rust = HashMap::new(); + + for (py_key, py_value) in imports_by_module_py.iter() { + let module: Module = py_key.extract().unwrap(); + let py_set = py_value + .downcast::() + .expect("Expected value to be a Python set."); + let mut hashset: HashSet = HashSet::new(); + for element in py_set.iter() { + let direct_import: DirectImport = element + .extract() + .expect("Expected value to be DirectImport."); + hashset.insert(direct_import); + } + imports_by_module_rust.insert(module, hashset); + } + + imports_by_module_rust } #[allow(unused_variables)] fn serialize_imports_by_module( imports_by_module: &HashMap>, ) -> String { - "".to_string() + let raw_map: HashMap<&str, Vec<(&str, usize, &str)>> = imports_by_module + .iter() + .map(|(module, imports)| { + let imports_vec: Vec<(&str, usize, &str)> = imports + .iter() + .map(|import| { + ( + import.imported.as_str(), + import.line_number, + import.line_contents.as_str(), + ) + }) + .collect(); + (module.name.as_str(), imports_vec) + }) + .collect(); + + serde_json::to_string(&raw_map).expect("Failed to serialize to JSON") } pub fn parse_json_to_map( diff --git a/rust/src/import_scanning.rs b/rust/src/import_scanning.rs index 0719c035..045ce411 100644 --- a/rust/src/import_scanning.rs +++ b/rust/src/import_scanning.rs @@ -18,6 +18,22 @@ pub struct DirectImport { pub line_contents: String, } +impl<'py> FromPyObject<'py> for DirectImport { + fn extract_bound(ob: &Bound<'py, PyAny>) -> PyResult { + let importer: String = ob.getattr("importer")?.getattr("name")?.extract()?; + let imported: String = ob.getattr("imported")?.getattr("name")?.extract()?; + let line_number: usize = ob.getattr("line_number")?.extract()?; + let line_contents: String = ob.getattr("line_contents")?.extract()?; + + Ok(DirectImport { + importer, + imported, + line_number, + line_contents, + }) + } +} + pub fn py_found_packages_to_rust(py_found_packages: &Bound<'_, PyAny>) -> HashSet { let py_set = py_found_packages .downcast::() diff --git a/rust/src/module_finding.rs b/rust/src/module_finding.rs index 4034b265..b1262697 100644 --- a/rust/src/module_finding.rs +++ b/rust/src/module_finding.rs @@ -20,6 +20,7 @@ impl fmt::Display for Module { } } + #[derive(Debug, Clone, PartialEq, Eq, Hash)] /// Set of modules found under a single package, together with metadata. pub struct FoundPackage { From 7e0d5bc7ac698ec60a019587e246a0bbf3b411a8 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Nov 2025 16:23:41 +0100 Subject: [PATCH 05/16] Fix FakeBasicFileSystem state sharing across clones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The FakeBasicFileSystem was using Box to store its data. When get_file_system_boxed() cloned the filesystem, it created a completely separate copy of the HashMap. This meant writes to the cloned filesystem were not visible in the original, causing test failures where cached files appeared to disappear after being written. Changed FakeBasicFileSystem to use Arc> instead, allowing all clones to share the same underlying data with synchronized access. This ensures writes made through any clone are visible to all references. Fixes the test_write_to_cache test failure described in PR #251. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- rust/src/filesystem.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/rust/src/filesystem.rs b/rust/src/filesystem.rs index a927e442..9ba98c56 100644 --- a/rust/src/filesystem.rs +++ b/rust/src/filesystem.rs @@ -8,7 +8,7 @@ use std::fs; use std::fs::File; use std::io::prelude::*; use std::path::{Path, PathBuf}; -use std::sync::LazyLock; +use std::sync::{Arc, LazyLock, Mutex}; use unindent::unindent; static ENCODING_RE: LazyLock = @@ -181,7 +181,7 @@ type FileSystemContents = HashMap; #[derive(Clone)] pub struct FakeBasicFileSystem { - contents: Box, + contents: Arc>, } // Implements BasicFileSystem (defined in grimp.application.ports.filesystem.BasicFileSystem). @@ -204,7 +204,7 @@ impl FakeBasicFileSystem { parsed_contents.extend(unindented_map); }; Ok(FakeBasicFileSystem { - contents: Box::new(parsed_contents), + contents: Arc::new(Mutex::new(parsed_contents)), }) } } @@ -246,13 +246,14 @@ impl FileSystem for FakeBasicFileSystem { /// Checks if a file or directory exists within the file system. fn exists(&self, file_name: &str) -> bool { - self.contents.contains_key(file_name) + self.contents.lock().unwrap().contains_key(file_name) } fn read(&self, file_name: &str) -> PyResult { - eprintln!("{:?}", &self.contents.keys()); - match self.contents.get(file_name) { - Some(file_name) => Ok(file_name.clone()), + let contents = self.contents.lock().unwrap(); + eprintln!("{:?}", contents.keys().collect::>()); + match contents.get(file_name) { + Some(file_contents) => Ok(file_contents.clone()), None => Err(PyFileNotFoundError::new_err(format!( "No such file: {file_name}" ))), @@ -262,10 +263,10 @@ impl FileSystem for FakeBasicFileSystem { #[allow(unused_variables)] fn write(&mut self, file_name: &str, contents: &str) -> PyResult<()> { eprintln!("Writing into fake {}, {}", file_name, contents); - eprintln!("Contents currently {:?}", self.contents); - self.contents - .insert(file_name.to_string(), contents.to_string()); - eprintln!("Contents now {:?}", self.contents); + let mut contents_mut = self.contents.lock().unwrap(); + eprintln!("Contents currently {:?}", *contents_mut); + contents_mut.insert(file_name.to_string(), contents.to_string()); + eprintln!("Contents now {:?}", *contents_mut); Ok(()) } } From 07b673331db0f1781082e9bab03a5064ee64f88e Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Nov 2025 16:28:25 +0100 Subject: [PATCH 06/16] Uncomment cache tests --- tests/unit/adaptors/test_caching.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/adaptors/test_caching.py b/tests/unit/adaptors/test_caching.py index 96454f3f..af4648b4 100644 --- a/tests/unit/adaptors/test_caching.py +++ b/tests/unit/adaptors/test_caching.py @@ -557,15 +557,15 @@ def test_uses_cache_multiple_packages( "cache_dir", ( "/tmp/some-cache-dir", - # "/tmp/some-cache-dir/", - # None, + "/tmp/some-cache-dir/", + None, ), ) @pytest.mark.parametrize( "include_external_packages, expected_data_file_name", ( (False, "blue,green.data.json"), - # (True, "blue,green:external.data.json"), + (True, "blue,green:external.data.json"), ), ) def test_write_to_cache( From 7517557669915046cac653c232fb7e06d1b6de0a Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Nov 2025 16:36:19 +0100 Subject: [PATCH 07/16] Small fixes --- rust/src/caching.rs | 3 +-- rust/src/module_finding.rs | 1 - src/grimp/adaptors/caching.py | 21 +-------------------- 3 files changed, 2 insertions(+), 23 deletions(-) diff --git a/rust/src/caching.rs b/rust/src/caching.rs index e2abf021..3f7f7330 100644 --- a/rust/src/caching.rs +++ b/rust/src/caching.rs @@ -7,7 +7,6 @@ use pyo3::types::{PyDict, PySet}; use pyo3::types::{PyDictMethods, PySetMethods}; use pyo3::{Bound, PyAny, PyResult, Python, pyfunction}; use std::collections::{HashMap, HashSet}; -use serde_json; /// Writes the cache file containing all the imports for a given package. /// Args: @@ -16,7 +15,7 @@ use serde_json; /// - file_system: The file system interface to use. (A BasicFileSystem.) #[pyfunction] pub fn write_cache_data_map_file<'py>( - py: Python<'py>, + _py: Python<'py>, filename: &str, imports_by_module: Bound<'py, PyDict>, file_system: Bound<'py, PyAny>, diff --git a/rust/src/module_finding.rs b/rust/src/module_finding.rs index b1262697..4034b265 100644 --- a/rust/src/module_finding.rs +++ b/rust/src/module_finding.rs @@ -20,7 +20,6 @@ impl fmt::Display for Module { } } - #[derive(Debug, Clone, PartialEq, Eq, Hash)] /// Set of modules found under a single package, together with metadata. pub struct FoundPackage { diff --git a/src/grimp/adaptors/caching.py b/src/grimp/adaptors/caching.py index dbfcfa4b..36310272 100644 --- a/src/grimp/adaptors/caching.py +++ b/src/grimp/adaptors/caching.py @@ -120,6 +120,7 @@ def write( self, imports_by_module: dict[Module, set[DirectImport]], ) -> None: + self._write_marker_files_if_not_already_there() # Write data file. data_cache_filename = self.file_system.join( self.cache_dir, @@ -129,31 +130,11 @@ def write( exclude_type_checking_imports=self.exclude_type_checking_imports, ), ) - # Rust version rust.write_cache_data_map_file( filename=data_cache_filename, imports_by_module=imports_by_module, file_system=self.file_system, ) - self._write_marker_files_if_not_already_there() - # Python version - primitives_map: PrimitiveFormat = {} - for found_package in self.found_packages: - primitives_map_for_found_package: PrimitiveFormat = { - module_file.module.name: [ - ( - direct_import.imported.name, - direct_import.line_number, - direct_import.line_contents, - ) - for direct_import in imports_by_module[module_file.module] - ] - for module_file in found_package.module_files - } - primitives_map.update(primitives_map_for_found_package) - - serialized = json.dumps(primitives_map) - # self.file_system.write(data_cache_filename, serialized) logger.info(f"Wrote data cache file {data_cache_filename}.") From f5743c194d8e9ad76606e3d2d6e3c9cfbd17d54f Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Nov 2025 17:46:03 +0100 Subject: [PATCH 08/16] Fix RealBasicFileSystem.write Make sure we create any needed parent directories. This is consistent with the python FileSystem implementation. --- rust/src/filesystem.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/rust/src/filesystem.rs b/rust/src/filesystem.rs index 9ba98c56..22cd00e0 100644 --- a/rust/src/filesystem.rs +++ b/rust/src/filesystem.rs @@ -135,7 +135,11 @@ impl FileSystem for RealBasicFileSystem { } fn write(&mut self, file_name: &str, contents: &str) -> PyResult<()> { - let mut file = File::create(file_name)?; + let file_path: PathBuf = file_name.into(); + if let Some(patent_dir) = file_path.parent() { + fs::create_dir_all(patent_dir)?; + } + let mut file = File::create(file_path)?; file.write_all(contents.as_bytes())?; Ok(()) } From 23f35b1fae9744f8617cdcea2daf4748a9a51cf7 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Nov 2025 17:47:29 +0100 Subject: [PATCH 09/16] Remove eprint! statements Pretty sure these were just added for debugging. --- rust/src/caching.rs | 2 -- rust/src/filesystem.rs | 5 ----- 2 files changed, 7 deletions(-) diff --git a/rust/src/caching.rs b/rust/src/caching.rs index 3f7f7330..2e584857 100644 --- a/rust/src/caching.rs +++ b/rust/src/caching.rs @@ -20,7 +20,6 @@ pub fn write_cache_data_map_file<'py>( imports_by_module: Bound<'py, PyDict>, file_system: Bound<'py, PyAny>, ) -> PyResult<()> { - eprintln!("About to clone for write."); let mut file_system_boxed = get_file_system_boxed(&file_system)?; let imports_by_module_rust = imports_by_module_to_rust(imports_by_module); @@ -43,7 +42,6 @@ pub fn read_cache_data_map_file<'py>( filename: &str, file_system: Bound<'py, PyAny>, ) -> PyResult> { - eprintln!("About to clone for read."); let file_system_boxed = get_file_system_boxed(&file_system)?; let file_contents = file_system_boxed.read(filename)?; diff --git a/rust/src/filesystem.rs b/rust/src/filesystem.rs index 22cd00e0..8e9b8f81 100644 --- a/rust/src/filesystem.rs +++ b/rust/src/filesystem.rs @@ -255,7 +255,6 @@ impl FileSystem for FakeBasicFileSystem { fn read(&self, file_name: &str) -> PyResult { let contents = self.contents.lock().unwrap(); - eprintln!("{:?}", contents.keys().collect::>()); match contents.get(file_name) { Some(file_contents) => Ok(file_contents.clone()), None => Err(PyFileNotFoundError::new_err(format!( @@ -266,11 +265,8 @@ impl FileSystem for FakeBasicFileSystem { #[allow(unused_variables)] fn write(&mut self, file_name: &str, contents: &str) -> PyResult<()> { - eprintln!("Writing into fake {}, {}", file_name, contents); let mut contents_mut = self.contents.lock().unwrap(); - eprintln!("Contents currently {:?}", *contents_mut); contents_mut.insert(file_name.to_string(), contents.to_string()); - eprintln!("Contents now {:?}", *contents_mut); Ok(()) } } @@ -415,7 +411,6 @@ pub fn get_file_system_boxed<'py>( file_system: &Bound<'py, PyAny>, ) -> PyResult> { let file_system_boxed: Box; - eprintln!("Cloning file system."); if let Ok(py_real) = file_system.extract::>() { file_system_boxed = Box::new(py_real.inner.clone()); } else if let Ok(py_fake) = file_system.extract::>() { From ec5c4f468d5e6d0e14f290868152b2bb8ec6585d Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 5 Nov 2025 07:22:50 +0100 Subject: [PATCH 10/16] Use rust.FakeBasicFileSystem directly in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace FakeFileSystem().convert_to_basic() with rust.FakeBasicFileSystem() to eliminate unnecessary Python wrapper layer. Addresses PR comment: https://github.com/python-grimp/grimp/pull/259#discussion_r2486373614 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- tests/unit/adaptors/test_caching.py | 32 ++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/unit/adaptors/test_caching.py b/tests/unit/adaptors/test_caching.py index af4648b4..3f6576db 100644 --- a/tests/unit/adaptors/test_caching.py +++ b/tests/unit/adaptors/test_caching.py @@ -7,7 +7,7 @@ from grimp.application.ports.caching import CacheMiss from grimp.application.ports.modulefinder import FoundPackage, ModuleFile from grimp.domain.valueobjects import DirectImport, Module -from tests.adaptors.filesystem import FakeFileSystem +from grimp import _rustgrimp as rust # type: ignore[attr-defined] class SimplisticFileNamer(CacheFileNamer): @@ -124,7 +124,7 @@ def test_make_data_file_unique_string( class TestCache: SOME_MTIME = 1676645081.4935088 - FILE_SYSTEM = FakeFileSystem( + FILE_SYSTEM = rust.FakeBasicFileSystem( contents=""" .grimp_cache/ mypackage.meta.json @@ -202,7 +202,7 @@ class TestCache: ] }""", }, - ).convert_to_basic() + ) MODULE_FILE_UNMODIFIED = ModuleFile( module=Module("mypackage.foo.unmodified"), mtime=SOME_MTIME ) @@ -249,7 +249,7 @@ def test_logs_missing_cache_files(self, caplog): caplog.set_level(logging.INFO, logger=Cache.__module__) Cache.setup( - file_system=FakeFileSystem().convert_to_basic(), # No cache files. + file_system=rust.FakeBasicFileSystem(), # No cache files. found_packages=self.FOUND_PACKAGES, namer=SimplisticFileNamer, include_external_packages=False, @@ -264,7 +264,7 @@ def test_logs_missing_cache_files(self, caplog): def test_logs_corrupt_cache_meta_file_reading(self, serialized_mtime: str, caplog): caplog.set_level(logging.WARNING, logger=Cache.__module__) - file_system = FakeFileSystem( + file_system = rust.FakeBasicFileSystem( contents=""" .grimp_cache/ mypackage.meta.json @@ -277,7 +277,7 @@ def test_logs_corrupt_cache_meta_file_reading(self, serialized_mtime: str, caplo }}""", ".grimp_cache/mypackage.data.json": "{}", }, - ).convert_to_basic() + ) Cache.setup( file_system=file_system, found_packages=self.FOUND_PACKAGES, @@ -292,7 +292,7 @@ def test_logs_corrupt_cache_meta_file_reading(self, serialized_mtime: str, caplo def test_logs_corrupt_cache_data_file_reading(self, caplog): caplog.set_level(logging.WARNING, logger=Cache.__module__) - file_system = FakeFileSystem( + file_system = rust.FakeBasicFileSystem( contents=""" .grimp_cache/ mypackage.meta.json @@ -305,7 +305,7 @@ def test_logs_corrupt_cache_data_file_reading(self, caplog): }}""", ".grimp_cache/mypackage.data.json": "INVALID JSON", }, - ).convert_to_basic() + ) Cache.setup( file_system=file_system, @@ -392,7 +392,7 @@ def test_uses_cache_for_module_with_same_mtime( ) def test_raises_cache_miss_for_missing_module_from_data(self): - file_system = FakeFileSystem( + file_system = rust.FakeBasicFileSystem( contents=""" .grimp_cache/ mypackage.meta.json @@ -407,7 +407,7 @@ def test_raises_cache_miss_for_missing_module_from_data(self): }}""", ".grimp_cache/mypackage.data.json": """{}""", }, - ).convert_to_basic() + ) module_file = ModuleFile(module=Module("mypackage.somemodule"), mtime=self.SOME_MTIME) cache = Cache.setup( file_system=file_system, @@ -427,7 +427,7 @@ def test_raises_cache_miss_for_missing_module_from_data(self): @pytest.mark.parametrize("serialized_mtime", ("INVALID_JSON", '["wrong", "type"]')) def test_raises_cache_miss_for_corrupt_meta_file(self, serialized_mtime): - file_system = FakeFileSystem( + file_system = rust.FakeBasicFileSystem( contents=""" .grimp_cache/ mypackage.meta.json @@ -450,7 +450,7 @@ def test_raises_cache_miss_for_corrupt_meta_file(self, serialized_mtime): ] }""", }, - ).convert_to_basic() + ) cache = Cache.setup( file_system=file_system, found_packages=self.FOUND_PACKAGES, @@ -463,7 +463,7 @@ def test_raises_cache_miss_for_corrupt_meta_file(self, serialized_mtime): @pytest.mark.parametrize("serialized_import", ("INVALID_JSON", '["wrong", "type"]')) def test_raises_cache_miss_for_corrupt_data_file(self, serialized_import): - file_system = FakeFileSystem( + file_system = rust.FakeBasicFileSystem( contents=""" .grimp_cache/ mypackage.meta.json @@ -486,7 +486,7 @@ def test_raises_cache_miss_for_corrupt_data_file(self, serialized_import): ] }}""", }, - ).convert_to_basic() + ) cache = Cache.setup( file_system=file_system, found_packages=self.FOUND_PACKAGES, @@ -572,7 +572,7 @@ def test_write_to_cache( self, include_external_packages, expected_data_file_name, cache_dir, caplog ): caplog.set_level(logging.INFO, logger=Cache.__module__) - file_system = FakeFileSystem().convert_to_basic() + file_system = rust.FakeBasicFileSystem() blue_one = Module(name="blue.one") blue_two = Module(name="blue.two") green_one = Module(name="green.one") @@ -679,7 +679,7 @@ def test_write_to_cache( def test_write_to_cache_adds_marker_files(self): some_cache_dir = "/tmp/some-cache-dir" - file_system = FakeFileSystem().convert_to_basic() + file_system = rust.FakeBasicFileSystem() cache = Cache.setup( file_system=file_system, cache_dir=some_cache_dir, From 0bbe668d279424d9133d688eded59abd95c2cdbd Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 5 Nov 2025 07:36:52 +0100 Subject: [PATCH 11/16] Make _scan_packages take BasicFileSystem directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change _scan_packages to accept BasicFileSystem parameter instead of AbstractFileSystem, and remove the .convert_to_basic() call inside the function. The conversion is now done in build_graph before calling _scan_packages, making the function signature match what it actually needs. Addresses PR comment: https://github.com/python-grimp/grimp/pull/259#discussion_r2486384546 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/grimp/application/usecases.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/grimp/application/usecases.py b/src/grimp/application/usecases.py index fb228770..e7503d29 100644 --- a/src/grimp/application/usecases.py +++ b/src/grimp/application/usecases.py @@ -7,7 +7,7 @@ from .scanning import scan_imports from ..application.ports import caching -from ..application.ports.filesystem import AbstractFileSystem +from ..application.ports.filesystem import AbstractFileSystem, BasicFileSystem from ..application.graph import ImportGraph from ..application.ports.modulefinder import AbstractModuleFinder, FoundPackage, ModuleFile from ..application.ports.packagefinder import AbstractPackageFinder @@ -57,7 +57,7 @@ def build_graph( imports_by_module = _scan_packages( found_packages=found_packages, - file_system=file_system, + file_system=file_system.convert_to_basic(), include_external_packages=include_external_packages, exclude_type_checking_imports=exclude_type_checking_imports, cache_dir=cache_dir, @@ -102,7 +102,7 @@ def _validate_package_names_are_strings( def _scan_packages( found_packages: set[FoundPackage], - file_system: AbstractFileSystem, + file_system: BasicFileSystem, include_external_packages: bool, exclude_type_checking_imports: bool, cache_dir: str | type[NotSupplied] | None, @@ -110,7 +110,7 @@ def _scan_packages( if cache_dir is not None: cache_dir_if_supplied = cache_dir if cache_dir != NotSupplied else None cache: caching.Cache = settings.CACHE_CLASS.setup( - file_system=file_system.convert_to_basic(), + file_system=file_system, found_packages=found_packages, include_external_packages=include_external_packages, exclude_type_checking_imports=exclude_type_checking_imports, From 91237efaa85235f0a0c477977b09c34af078f65c Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 5 Nov 2025 07:40:16 +0100 Subject: [PATCH 12/16] Remove unused _py parameter from write_cache_data_map_file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Python GIL token parameter was not being used in the function body, so it has been removed to simplify the function signature. Addresses PR comment: https://github.com/python-grimp/grimp/pull/259#discussion_r2486397822 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- rust/src/caching.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/rust/src/caching.rs b/rust/src/caching.rs index 2e584857..2ce9d9e4 100644 --- a/rust/src/caching.rs +++ b/rust/src/caching.rs @@ -15,7 +15,6 @@ use std::collections::{HashMap, HashSet}; /// - file_system: The file system interface to use. (A BasicFileSystem.) #[pyfunction] pub fn write_cache_data_map_file<'py>( - _py: Python<'py>, filename: &str, imports_by_module: Bound<'py, PyDict>, file_system: Bound<'py, PyAny>, From 527a058d9f7854bce84e99f2f4cfa22bbc9a147d Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 5 Nov 2025 08:07:30 +0100 Subject: [PATCH 13/16] Remove unnecessary #[allow(unused_variables)] attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The attributes were not needed as all variables in these functions are actually being used. The code compiles and passes all tests without these attributes. Addresses PR comment: https://github.com/python-grimp/grimp/pull/259#discussion_r2486402706 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- rust/src/caching.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/rust/src/caching.rs b/rust/src/caching.rs index 2ce9d9e4..2e7b2e13 100644 --- a/rust/src/caching.rs +++ b/rust/src/caching.rs @@ -50,7 +50,6 @@ pub fn read_cache_data_map_file<'py>( Ok(imports_by_module_to_py(py, imports_by_module)) } -#[allow(unused_variables)] fn imports_by_module_to_rust( imports_by_module_py: Bound, ) -> HashMap> { @@ -74,7 +73,6 @@ fn imports_by_module_to_rust( imports_by_module_rust } -#[allow(unused_variables)] fn serialize_imports_by_module( imports_by_module: &HashMap>, ) -> String { From a25e6df37a4b81e76371c83a2362c612ca6a5fcb Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 5 Nov 2025 08:13:14 +0100 Subject: [PATCH 14/16] Simplify write method by chaining operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Combine File::create() and write_all() into a single chained expression without intermediate variables, and use map_err for error conversion. This results in more concise and idiomatic Rust code. Addresses PR comment: https://github.com/python-grimp/grimp/pull/259#discussion_r2486635845 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- rust/src/filesystem.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/src/filesystem.rs b/rust/src/filesystem.rs index 8e9b8f81..e12b547c 100644 --- a/rust/src/filesystem.rs +++ b/rust/src/filesystem.rs @@ -139,9 +139,9 @@ impl FileSystem for RealBasicFileSystem { if let Some(patent_dir) = file_path.parent() { fs::create_dir_all(patent_dir)?; } - let mut file = File::create(file_path)?; - file.write_all(contents.as_bytes())?; - Ok(()) + File::create(file_path)? + .write_all(contents.as_bytes()) + .map_err(Into::into) } } From c8e353a40d10db92c642327552b8337be6f1eec9 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 5 Nov 2025 08:42:58 +0100 Subject: [PATCH 15/16] Implement FromPyObject for ImportsByModule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the imports_by_module_to_rust helper function with a more idiomatic Rust approach by implementing FromPyObject trait for an ImportsByModule newtype wrapper. This allows using .extract() directly, making the code more ergonomic and following Rust best practices similar to DirectImport. The new ImportsByModule wrapper encapsulates HashMap> and provides automatic conversion from Python dict objects. Addresses PR comment: https://github.com/python-grimp/grimp/pull/259#discussion_r2486413276 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- rust/src/caching.rs | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/rust/src/caching.rs b/rust/src/caching.rs index 2e7b2e13..84a64bad 100644 --- a/rust/src/caching.rs +++ b/rust/src/caching.rs @@ -5,7 +5,7 @@ use crate::module_finding::Module; use pyo3::types::PyAnyMethods; use pyo3::types::{PyDict, PySet}; use pyo3::types::{PyDictMethods, PySetMethods}; -use pyo3::{Bound, PyAny, PyResult, Python, pyfunction}; +use pyo3::{Bound, FromPyObject, PyAny, PyResult, Python, pyfunction}; use std::collections::{HashMap, HashSet}; /// Writes the cache file containing all the imports for a given package. @@ -21,7 +21,7 @@ pub fn write_cache_data_map_file<'py>( ) -> PyResult<()> { let mut file_system_boxed = get_file_system_boxed(&file_system)?; - let imports_by_module_rust = imports_by_module_to_rust(imports_by_module); + let ImportsByModule(imports_by_module_rust) = imports_by_module.extract()?; let file_contents = serialize_imports_by_module(&imports_by_module_rust); @@ -50,27 +50,27 @@ pub fn read_cache_data_map_file<'py>( Ok(imports_by_module_to_py(py, imports_by_module)) } -fn imports_by_module_to_rust( - imports_by_module_py: Bound, -) -> HashMap> { - let mut imports_by_module_rust = HashMap::new(); - - for (py_key, py_value) in imports_by_module_py.iter() { - let module: Module = py_key.extract().unwrap(); - let py_set = py_value - .downcast::() - .expect("Expected value to be a Python set."); - let mut hashset: HashSet = HashSet::new(); - for element in py_set.iter() { - let direct_import: DirectImport = element - .extract() - .expect("Expected value to be DirectImport."); - hashset.insert(direct_import); +/// A newtype wrapper for HashMap> that implements FromPyObject. +pub struct ImportsByModule(pub HashMap>); + +impl<'py> FromPyObject<'py> for ImportsByModule { + fn extract_bound(ob: &Bound<'py, PyAny>) -> PyResult { + let py_dict = ob.downcast::()?; + let mut imports_by_module_rust = HashMap::new(); + + for (py_key, py_value) in py_dict.iter() { + let module: Module = py_key.extract()?; + let py_set = py_value.downcast::()?; + let mut hashset: HashSet = HashSet::new(); + for element in py_set.iter() { + let direct_import: DirectImport = element.extract()?; + hashset.insert(direct_import); + } + imports_by_module_rust.insert(module, hashset); } - imports_by_module_rust.insert(module, hashset); - } - imports_by_module_rust + Ok(ImportsByModule(imports_by_module_rust)) + } } fn serialize_imports_by_module( From 6e854ecffa693e7301896844e024acfa61e5830b Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 5 Nov 2025 09:11:54 +0100 Subject: [PATCH 16/16] Make internal types and functions private MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make implementation details private to reduce the public API surface: In filesystem.rs: - RealBasicFileSystem and FakeBasicFileSystem structs - Their inner fields - parse_indented_file_system_string function In import_scanning.rs: - py_found_packages_to_rust function - get_modules_from_found_packages function - scan_for_imports_no_py function - to_py_direct_imports function Only the Python-facing wrapper structs (PyRealBasicFileSystem, PyFakeBasicFileSystem) and the exported Python functions remain public. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- rust/src/filesystem.rs | 10 +++++----- rust/src/import_scanning.rs | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/rust/src/filesystem.rs b/rust/src/filesystem.rs index e12b547c..c234aa64 100644 --- a/rust/src/filesystem.rs +++ b/rust/src/filesystem.rs @@ -30,13 +30,13 @@ pub trait FileSystem: Send + Sync { #[derive(Clone)] #[pyclass] -pub struct RealBasicFileSystem {} +struct RealBasicFileSystem {} // Implements a BasicFileSystem (defined in grimp.application.ports.filesystem.BasicFileSystem) // that actually reads files. #[pyclass(name = "RealBasicFileSystem")] pub struct PyRealBasicFileSystem { - pub inner: RealBasicFileSystem, + inner: RealBasicFileSystem, } impl FileSystem for RealBasicFileSystem { @@ -184,14 +184,14 @@ impl PyRealBasicFileSystem { type FileSystemContents = HashMap; #[derive(Clone)] -pub struct FakeBasicFileSystem { +struct FakeBasicFileSystem { contents: Arc>, } // Implements BasicFileSystem (defined in grimp.application.ports.filesystem.BasicFileSystem). #[pyclass(name = "FakeBasicFileSystem")] pub struct PyFakeBasicFileSystem { - pub inner: FakeBasicFileSystem, + inner: FakeBasicFileSystem, } impl FakeBasicFileSystem { @@ -319,7 +319,7 @@ impl PyFakeBasicFileSystem { /// Parses an indented string representing a file system structure /// into a HashMap where keys are full file paths. /// See tests.adaptors.filesystem.FakeFileSystem for the API. -pub fn parse_indented_file_system_string(file_system_string: &str) -> HashMap { +fn parse_indented_file_system_string(file_system_string: &str) -> HashMap { let mut file_paths_map: HashMap = HashMap::new(); let mut path_stack: Vec = Vec::new(); // Stores current directory path components let mut first_line = true; // Flag to handle the very first path component diff --git a/rust/src/import_scanning.rs b/rust/src/import_scanning.rs index 045ce411..dee2a420 100644 --- a/rust/src/import_scanning.rs +++ b/rust/src/import_scanning.rs @@ -34,7 +34,7 @@ impl<'py> FromPyObject<'py> for DirectImport { } } -pub fn py_found_packages_to_rust(py_found_packages: &Bound<'_, PyAny>) -> HashSet { +fn py_found_packages_to_rust(py_found_packages: &Bound<'_, PyAny>) -> HashSet { let py_set = py_found_packages .downcast::() .expect("Expected py_found_packages to be a Python set."); @@ -52,7 +52,7 @@ pub fn py_found_packages_to_rust(py_found_packages: &Bound<'_, PyAny>) -> HashSe rust_found_packages } -pub fn get_modules_from_found_packages(found_packages: &HashSet) -> HashSet { +fn get_modules_from_found_packages(found_packages: &HashSet) -> HashSet { let mut modules = HashSet::new(); for package in found_packages { for module_file in &package.module_files { @@ -73,7 +73,7 @@ fn module_is_descendant(module_name: &str, potential_ancestor: &str) -> bool { /// Statically analyses the given module and returns a set of Modules that /// it imports. #[allow(clippy::borrowed_box)] -pub fn scan_for_imports_no_py( +fn scan_for_imports_no_py( file_system: &Box, found_packages: &HashSet, include_external_packages: bool, @@ -169,7 +169,7 @@ fn scan_for_imports_no_py_single_module( Ok(imports) } -pub fn to_py_direct_imports<'a>( +fn to_py_direct_imports<'a>( py: Python<'a>, rust_imports: &HashSet, ) -> Bound<'a, PySet> {