Add module expressions by Peter554 · Pull Request #188 · python-grimp/grimp

Peter554 · 2025-02-13T23:43:34Z

This PR adds module expressions to grimp. By moving module expressions into grimp, in particular into the rust code, we will be able to speed up lots of the import linter code that works with module expressions (import linter now supports module expressions in quite a few places).

codspeed-hq · 2025-02-14T00:00:13Z

CodSpeed Performance Report

Merging #188 will not alter performance

_{Comparing Peter554:module-expressions (eb43939) with master (8c1bb5d)}

Summary

✅ 17 untouched benchmarks
🆕 2 new benchmarks

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
🆕	`test_find_matching_direct_imports`	N/A	36.1 ms	N/A
🆕	`test_find_matching_modules`	N/A	9.2 ms	N/A

docs/usage.rst

rust/src/graph/hierarchy_queries.rs

Peter554 · 2025-02-22T06:30:16Z

tests/unit/adaptors/graph/test_hierarchy.py

+    def test_finds_matching_modules(self):
+        graph = ImportGraph()
+        graph.add_module("foo")
+        graph.add_module("foo.bar")
+        graph.add_module("foo.bar.baz")
+
+        assert graph.find_matching_modules("foo") == {"foo"}
+        assert graph.find_matching_modules("foo.*") == {"foo.bar"}
+        assert graph.find_matching_modules("foo.**") == {"foo.bar", "foo.bar.baz"}


🍷 This test basically duplicates the rust test. For me it's okay to keep both.

I think a good rule of thumb is that the Python tests should be comprehensive, and if we want to add some Rust tests to help too (even if they duplicate) then that's fine. Do you agree?

Do you agree?

I think I could be persuaded either way. In the sense of a testing pyramid, one might argue that the rust tests should be the most comprehensive, since this is the closest we have to a unit test. All the python tests integrate the rust code, so by definition are higher up the pyramid, so we'd expect less comprehensive tests there.

That said, this isn't a normal testing pyramid, and the preexisting code puts most of the tests in python so there is definitely precedent to follow your suggestion.

In summary I think it's good to have both, certainly with comprehensive python tests. I'll flesh out the python tests a bit more.

Thinking back to when we swapped in your Rust implementation, it was a really useful property of the test suite that there was thorough test coverage of the Python API - we just deleted all the Rust tests and still felt confident. If coverage had been split across the two languages then we wouldn't have been able to do that.

So I think focusing on the Python tests is worthwhile - we can treat the Rust tests more lightly as tests to help us as we go along, but more disposable.

Arguably the Python unit tests are still at the base of the testing pyramid - they're fast and give us lots of signal about granular behaviour.

Yes okay I think you're right 👍

src/grimp/application/ports/graph.py

rust/src/graph/direct_import_queries.rs

tests/unit/adaptors/graph/test_direct_imports.py

src/grimp/application/ports/graph.py

rust/src/graph/pathfinding.rs

seddonym

Just leaving some early feedback - I haven't yet gone over everything in detail.

seddonym · 2025-02-24T14:26:05Z

tests/unit/adaptors/graph/test_hierarchy.py

+        graph.add_module("foo.bar")
+        graph.add_module("foo.bar.baz")
+
+        assert graph.find_matching_modules("foo") == {"foo"}


My immediate reaction to the Python tests is they don't seem as thorough as what we have in import linter (i.e. here and here). But I see there are also Rust tests. How close do you think we are to equivalent test coverage as in Import Linter?

I'm reasonably confident that we have similar coverage - the rust test cases within module_expressions.rs cover all of that I think. There aren't many tests at the level of find_matching_modules and find_matching_direct_imports. Most of the testing is done as unit tests on the ModuleExpression i.e. test_parse and test_is_match.

As per the other discussion, would you mind making sure the behaviour is covered by the Python tests, too? Or I can do it once everything else is ready on the PR.

src/grimp/application/ports/graph.py

src/grimp/exceptions.py

rust/src/graph/direct_import_queries.rs

src/grimp/application/ports/graph.py

seddonym · 2025-02-24T15:00:18Z

tests/unit/adaptors/graph/test_hierarchy.py

+    def test_finds_matching_modules(self):
+        graph = ImportGraph()
+        graph.add_module("foo")
+        graph.add_module("foo.bar")
+        graph.add_module("foo.bar.baz")
+
+        assert graph.find_matching_modules("foo") == {"foo"}
+        assert graph.find_matching_modules("foo.*") == {"foo.bar"}
+        assert graph.find_matching_modules("foo.**") == {"foo.bar", "foo.bar.baz"}


I think a good rule of thumb is that the Python tests should be comprehensive, and if we want to add some Rust tests to help too (even if they duplicate) then that's fine. Do you agree?

seddonym

Another round of comments.

The commits are on the large side for reviewing, by the way...

rust/src/module_expressions.rs

rust/src/graph/hierarchy_queries.rs

Peter554 · 2025-02-24T21:11:59Z

@seddonym I've spent some time working through your comments. I'm not sure if I quite got them all, there's quite a few - perhaps you can take a look and close off the ones you are happy with?

Remove the "bidirectional" from the function name, since this is the only path finding function we have. The "bidirectional" part is just an implementation detail.

seddonym

Ok I've finished the full review. It's looking great in general, thanks for your efforts!

I don't think there's much more to do on this, mainly it's just a few tweaks to comments.

Before merging to master, there are two blockers:

Making sure the Python test coverage is comprehensive.
Figuring out whether to represent import expressions as two-tuples or arrow-strings (let's discuss).

I'd be happy to merge this into a branch in my repo if it would help - then we could both work on the remainder. Let me know what you prefer.

rust/src/module_expressions.rs

docs/usage.rst

rust/src/graph/hierarchy_queries.rs

src/grimp/application/ports/graph.py

seddonym · 2025-02-25T09:02:35Z

tests/unit/adaptors/graph/test_hierarchy.py

+    def test_finds_matching_modules(self):
+        graph = ImportGraph()
+        graph.add_module("foo")
+        graph.add_module("foo.bar")
+        graph.add_module("foo.bar.baz")
+
+        assert graph.find_matching_modules("foo") == {"foo"}
+        assert graph.find_matching_modules("foo.*") == {"foo.bar"}
+        assert graph.find_matching_modules("foo.**") == {"foo.bar", "foo.bar.baz"}


Thinking back to when we swapped in your Rust implementation, it was a really useful property of the test suite that there was thorough test coverage of the Python API - we just deleted all the Rust tests and still felt confident. If coverage had been split across the two languages then we wouldn't have been able to do that.

So I think focusing on the Python tests is worthwhile - we can treat the Rust tests more lightly as tests to help us as we go along, but more disposable.

Arguably the Python unit tests are still at the base of the testing pyramid - they're fast and give us lots of signal about granular behaviour.

seddonym · 2025-02-25T09:04:24Z

tests/unit/adaptors/graph/test_hierarchy.py

+        graph.add_module("foo.bar")
+        graph.add_module("foo.bar.baz")
+
+        assert graph.find_matching_modules("foo") == {"foo"}


As per the other discussion, would you mind making sure the behaviour is covered by the Python tests, too? Or I can do it once everything else is ready on the PR.

rust/src/graph/direct_import_queries.rs

rust/src/module_expressions.rs

seddonym

Merging to the add-module-expressions working branch in the origin repo.

This was referenced Feb 13, 2025

POC Faster remove ignored imports seddonym/import-linter#253

Closed

Make remove_ignored_imports faster (when wildcard import expressions are used) seddonym/import-linter#252

Open

Peter554 force-pushed the module-expressions branch 5 times, most recently from c8456dd to 4ac6d87 Compare February 22, 2025 06:17

Peter554 marked this pull request as ready for review February 22, 2025 06:25