Closed layers #227

Peter554 · 2025-06-18T19:01:25Z

(AI assisted PR description)

Implement Closed Layers Feature

Overview

This PR introduces the concept of "closed layers" to Grimp, providing an additional constraint mechanism for controlling imports between modules in a Python codebase.

Key Behaviour

When a layer is marked as closed, modules in higher layers cannot import modules that exist below the closed layer. This creates a strict boundary that forces higher layers to interact only with the closed layer itself, not with anything beneath it. This is particularly useful for enforcing clean architectural boundaries where lower-level implementation details should be hidden from higher-level components.

Example Use Case

Consider this layered architecture:

app/
  high_level/     # Higher layer
  mid_level/      # Middle layer (closed)
  low_level/      # Lower layer

With mid_level configured as a closed layer:

high_level can import from mid_level
high_level cannot import from low_level (must go through mid_level)
mid_level can import from low_level

This enforces that high_level modules can only interact with low_level functionality through the mid_level interface, preventing dependency leakage and maintaining clear architectural boundaries.

codspeed-hq · 2025-06-18T19:07:41Z

CodSpeed Instrumentation Performance Report

Merging #227 will not alter performance

_{Comparing Peter554:closed-layers (5c6a9cf) with master (0abffb2)}

Summary

✅ 22 untouched benchmarks

Peter554 · 2025-06-19T10:35:37Z

rust/src/graph/higher_order_queries.rs

                    }
                }
+
+                // Should not be imported by higher layers if there is a closed layer inbetween.


@seddonym I'm unsure if this is the definition of closed layers that we want. Is this behaviour consistent with your understanding of the term "closed layer"?

The test_cannot_import_through_closed_mid makes the implemented behaviour clear.

Yes I think so. The question of whether to disallow indirect imports is interesting though, I wonder if that is something we would need to make optional. Let me have a think about it.

Coming back to it, I agree with how you've done it here. Can you see any reason why we should allow indirect imports?

I agree with you.

Can you see any reason why we should allow indirect imports?

I think allowing indirect imports would undermine the point of the contract. I think indirect imports should be disallowed. If we find that for some reason we need that in the future, then we could always add that option then (but by default I think indirect imports should be disallowed).

seddonym

Great to see this taking shape!

I haven't reviewed the implementation in detail yet, but I like what I see so far. I do think we should adjust it so that the field on the Layer is closed (default False), not open (default True). This is because I want closed layers to feel like more of a special case.

Happy to discuss if you feel strongly the other way.

src/grimp/domain/valueobjects.py

Peter554 · 2025-06-20T11:11:03Z

Looks like there is a related issue in import linter seddonym/import-linter#245

seddonym

This is great stuff. I have hardly anything to add, other than it would be good to sense check the behaviour when there are multiple closed layers - and add tests for that.

seddonym · 2025-07-11T16:06:19Z

src/grimp/domain/valueobjects.py


    def __str__(self) -> str:
-        return f"{self.module_tails}, independent={self.independent}"
+        return f"{self.module_tails}, independent={self.independent}, closed={self.closed}"


Could we have test coverage for this?

seddonym · 2025-07-11T16:12:23Z

CHANGELOG.rst

+Unreleased
+----------
+
+* Implement closed layers - a feature that prevents imports from higher layers


Could we change this to Add closed layers to layer contract. - and skip the rest?

seddonym · 2025-07-11T16:36:46Z

rust/src/graph/higher_order_queries.rs

+
+        let permutations = graph.generate_module_permutations(&levels);
+
+        // Modules in independent layer should not import each other


Out of interest, why not just do a single assertion on what the result is?

LLM generated it this way 😅 Updated now.

rust/src/graph/higher_order_queries.rs

seddonym · 2025-07-11T16:46:31Z

rust/src/graph/higher_order_queries.rs

+
+        let permutations = graph.generate_module_permutations(&levels);
+
+        assert_eq!(


Personally I think this would be clearer as a single assertion on the result.

♻️ #227 (comment)

seddonym · 2025-07-11T16:48:21Z

rust/src/graph/higher_order_queries.rs

                    }
                }
+
+                // Should not be imported by higher layers if there is a closed layer inbetween.


Coming back to it, I agree with how you've done it here. Can you see any reason why we should allow indirect imports?

seddonym · 2025-07-11T16:51:37Z

tests/unit/adaptors/graph/test_layers.py

    return zip(a, b)
+
+
+class TestClosedLayers:


I think we're missing tests for layers with multiple closed layers, right?

Maybe. I'm unsure exactly what behaviour we want to test here. I've added another unit test case for generate_module_permutations for two closed layers - does this cover what you were hoping to test?

docs/usage.rst

seddonym · 2025-07-11T16:53:55Z

rust/src/graph/higher_order_queries.rs

+
+        let levels = vec![top_level, middle_level, bottom_level];
+
+        let permutations = graph.generate_module_permutations(&levels);


Coming to this function afresh, makes me think it's not very well named (or at least could do with more documentation) - do you agree?

True - renamed

Peter554 · 2025-07-12T19:55:35Z

src/grimp/domain/valueobjects.py


    def __str__(self) -> str:
-        return f"{self.module_tails}, independent={self.independent}"
+        module_tails = sorted(self.module_tails)


This sorted needed to make the test stable.

The returned order isn't stable anyway, and order doesn't matter.

Peter554 · 2025-07-13T09:46:12Z

Hi @seddonym - this is ready for another look now. Sorry the commit history is a bit messy.

seddonym

Nearly there - I do think it's important that we have reasonably thorough Python tests for this though. Thanks for all your work on it so far!

seddonym · 2025-07-15T15:13:08Z

rust/src/graph/higher_order_queries.rs

    }

-    fn generate_module_permutations(
+    fn generate_illegal_import_permutations_for_layers(


Definitely better - might be worth adding a comment to explain what the tuple corresponds to (e.g. is the first one the source and the second one the module that should not be imported by it)?

Added a comment in the docstring now.

seddonym · 2025-07-15T15:15:05Z

rust/src/graph/higher_order_queries.rs

    }
+
+    #[test]
+    fn test_generate_module_permutations_two_closed_layers() {


I think we should have Python tests for this too (or instead of) - one of the principles of the project is that the Python tests are pretty comprehensive.

Okay sure - python tests extended now.

seddonym · 2025-07-15T15:16:27Z

rust/src/graph/higher_order_queries.rs

+
+        let top_level = Level::new(top_layer, false, false);
+        let middle_top_level = Level::new(middle_top_layer, false, true); // Closed
+        let middle_bottom_level = Level::new(middle_bottom_layer, false, true); // Closed


Good to have a test for two closed layers together, but feels like we could also do with something along these lines

one two three (closed) four five six (closed) seven eight

I'd put that in the Python test though probably.

♻️ #227 (comment)

I tested this case:

layers = [ Layer("highest"), Layer("high", closed=True), Layer("mid"), Layer("low", closed=True), Layer("lowest"), ]

I'm not sure what benefit we'd get from testing even more layers, so kept it simple (in your example you put two open layers "four" and "five" between the closed layers) 🤔

I was just thinking about testing boundaries, but yes this is much better now. Thank you!

seddonym

Looks great. Thanks so much for this!

seddonym · 2025-07-16T07:02:24Z

I imagine you will probably be thinking about adding this to Import Linter - and therefore when the next Grimp release will be.

I am considering delaying the release until I have done the follow up work from #229, but I could be talked around 😄.

Peter554 force-pushed the closed-layers branch from 9d6308a to befce13 Compare June 18, 2025 19:06

Peter554 force-pushed the closed-layers branch from befce13 to 66d3847 Compare June 18, 2025 19:08

Peter554 marked this pull request as ready for review June 18, 2025 19:09

Peter554 force-pushed the closed-layers branch 2 times, most recently from db30b9a to 2407f38 Compare June 19, 2025 10:26

Peter554 commented Jun 19, 2025

View reviewed changes

seddonym requested changes Jun 19, 2025

View reviewed changes

src/grimp/domain/valueobjects.py Outdated Show resolved Hide resolved

src/grimp/domain/valueobjects.py Outdated Show resolved Hide resolved

Peter554 force-pushed the closed-layers branch from 2407f38 to 106cd93 Compare June 19, 2025 17:30

Peter554 requested a review from seddonym June 19, 2025 17:32

Peter554 force-pushed the closed-layers branch from 106cd93 to b28e427 Compare June 20, 2025 09:39

Peter554 mentioned this pull request Jun 20, 2025

Closed layers seddonym/import-linter#245

Open

seddonym requested changes Jul 11, 2025

View reviewed changes

Peter554 force-pushed the closed-layers branch 3 times, most recently from 099420e to c30793d Compare July 12, 2025 19:54

Peter554 commented Jul 12, 2025

View reviewed changes

Peter554 added 8 commits July 12, 2025 21:57

Make clippy happy

e0c2985

Add unit tests for generate_module_permutations

b5be3ff

Add closed to Layer class

026fc00

Add closed to Level struct

ac5b197

Adapt generate_module_permutations

d096153

Add TestClosedLayers

252fe62

Update documentation

7362cf9

Update changelog

2f33fee

Peter554 force-pushed the closed-layers branch from c30793d to 2f33fee Compare July 12, 2025 19:57

Return hashset from generate_module_permutations

5b669fb

The returned order isn't stable anyway, and order doesn't matter.

Peter554 force-pushed the closed-layers branch from 9f8e05a to 4f10c12 Compare July 13, 2025 05:51

Peter554 requested a review from seddonym July 13, 2025 09:45

seddonym requested changes Jul 15, 2025

View reviewed changes

Rename generate_module_permutations

10d41fb

Peter554 force-pushed the closed-layers branch 2 times, most recently from b716fff to 7e2c3ae Compare July 16, 2025 05:13

Refactor and extend TestClosedLayers

5c6a9cf

Peter554 force-pushed the closed-layers branch from 7e2c3ae to 5c6a9cf Compare July 16, 2025 06:33

Peter554 requested a review from seddonym July 16, 2025 06:33

seddonym approved these changes Jul 16, 2025

View reviewed changes

seddonym merged commit 01d0098 into python-grimp:master Jul 16, 2025
18 checks passed


		let permutations = graph.generate_module_permutations(&levels);

		// Modules in independent layer should not import each other


		let permutations = graph.generate_module_permutations(&levels);

		assert_eq!(


		let levels = vec![top_level, middle_level, bottom_level];

		let permutations = graph.generate_module_permutations(&levels);

Closed layers #227

Closed layers #227

Uh oh!

Conversation

Peter554 commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implement Closed Layers Feature

Overview

Key Behaviour

Example Use Case

Uh oh!

codspeed-hq bot commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #227 will not alter performance

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Peter554 Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seddonym left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Peter554 commented Jun 20, 2025

Uh oh!

seddonym left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Peter554 Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Peter554 commented Jul 13, 2025

Uh oh!

seddonym left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Peter554 commented Jun 18, 2025 •

edited

Loading

codspeed-hq bot commented Jun 18, 2025 •

edited

Loading

Peter554 Jul 12, 2025 •

edited

Loading

Peter554 Jul 13, 2025 •

edited

Loading