Allow layer sibling independence to be configured #136

Peter554 · 2023-11-21T16:03:17Z

This PR contains the changes needed within grimp to support importlinter: Proposal: Allow configuring sibling independence.

The rust find_illegal_dependencies function is adapted to accept a tuple of level dicts, rather than a tuple of level sets. Each level dict contains the layers for that level (in a set) and the level dict also contains a boolean field independent which find_illegal_dependencies uses to determine whether the siblings within that layer should be independent.
The python graph.find_illegal_dependencies_for_layers method is extended to accept a Sequence[Union[str, set[str], Level]], where Level is a TypedDict matching the level dict expected by the rust find_illegal_dependencies function.
Existing behaviour is maintained - no breaking changes. A simple set of strings can still be passed to the python graph.find_illegal_dependencies_for_layers API, and these sibling layers will be treated as independent, as before.

rust/src/lib.rs

src/grimp/adaptors/_layers.py

Peter554 · 2023-11-28T20:35:53Z

@seddonym this is ready for review now. Hopefully it's along the right lines, if not then let's just have a quick chat.

seddonym · 2023-11-29T15:58:43Z

Thank you! Will take a look over the next few days.

Peter554 · 2023-11-29T20:27:57Z

Thank you! Will take a look over the next few days.

Thanks! PR seems to be closed though, was that intentional?

seddonym · 2023-11-29T21:19:26Z

Oops no not at all, sorry! Will reopen when at a computer

seddonym

This is fantastic work, thank you!

My only concern is the Python API, as per my comment. Would be good to hear your thoughts.

rust/src/layers.rs

seddonym · 2023-12-13T13:38:23Z

rust/src/lib.rs

@@ -47,15 +47,18 @@ pub fn find_illegal_dependencies<'a>(
    convert_dependencies_to_python(py, dependencies, &graph)
 }

-fn rustify_levels(levels_python: &PyTuple) -> Vec<Level> {
+fn rustify_levels(levels_python: &PyTuple) -> PyResult<Vec<Level>> {


The purpose of this function is to convert the Python objects into native rust types. I'd prefer we didn't change this - can we find another way? We could even do some validation in Python before passing it to Rust - I agree with you, we don't need to worry about panicking.

seddonym · 2023-12-13T13:40:53Z

tests/unit/adaptors/graph/test_layers.py

@@ -435,6 +438,24 @@ def test_direct_illegal_between_sibling_layers(
            ),
        }

+    def test_imports_between_sibling_layers_illegal_if_and_only_if_level_is_independent(self):


I think this test would be better as a parametrized test on independent=True or False, that way we get more signal on which case fails.

Sure, I've extended the existing test_direct_illegal_between_sibling_layers now.

src/grimp/adaptors/_layers.py

seddonym · 2023-12-13T13:55:17Z

src/grimp/application/ports/graph.py

@@ -15,6 +15,11 @@ class DetailedImport(TypedDict):
    line_contents: str


+class Level(TypedDict):


Could we have a docstring to explain this concept?

Yes, added.

seddonym · 2023-12-13T14:18:12Z

src/grimp/adaptors/_layers.py

@@ -52,11 +53,24 @@ class _RustPackageDependency(TypedDict):
    routes: tuple[_RustRoute, ...]


-def _layers_to_levels(layers: Sequence[Union[str, set[str]]]) -> tuple[set[str], ...]:
+def _layers_to_levels(layers: Sequence[Union[str, set[str], Level]]) -> tuple[Level, ...]:


This change to the API feels quite significant, and is the only thing that concerns me about this PR. As you've probably noticed, up until now the API just uses native Python datatypes and I think that has worked well generally, though it is being stretched a little with the more complex return types of find_illegal_dependencies_for_layers and get_import_details. I do really like the way we haven't needed any custom datatypes so far, but I'm starting to wonder if it's time to move beyond this as the API grows in complexity.

For me, passing it in as a dict is dict(independent=False, layers={"green", "blue"}) probably crosses a boundary: a custom datatype would be simpler here. That said, I do really like the approach of backward compatibility you've taken. I've been trying to think of alternatives.

One option (let's call it Option A) would be to define Level as a custom class (not just a TypedDict). The API might look like this:

dependencies = graph.find_illegal_dependencies_for_layers( layers=( "red", grimp.Level({"green", "blue"}, independent=False), "yellow", ), )

Notice independent is moved to the end and the layers have been made a positional argument, for terseness.

Another even terser option (Option B) would be to make all the layers positional so we don't need to pass them in as a set, i.e. grimp.Level("green", "blue", independent=False).

With either of these options, I think it would be nice to make Level the canonical datatype passed to the function, with the str and set[str] options being conveniences that are converted to Level objects straight away. So we could also call the function like this:

dependencies = graph.find_illegal_dependencies_for_layers( layers=( grimp.Level("red"), grimp.Level("green", "blue", independent=False), grimp.Level("yellow", "orange"), ), )

If we did that then it would make sense to annotate layers as Level | str | set[str], to make level more prominent. Or possibly we should even rename it to levels (we'd need to figure out backward compatibility though).

Another option (Option C) I considered was to use the decorator pattern just to wrap the set:

dependencies = graph.find_illegal_dependencies_for_layers( layers=( "red", grimp.dependent({"green", "blue"}), "yellow", ), )

This has the advantage of being very terse, though I do wonder if it is potentially confusing.

I'm leaning towards Option B, interested to hear your thoughts.

I was originally thinking to introduce a class for this, a bit like you're suggesting. I had decided I wouldn't mainly because of the internal rust function - when calling the rust function it seemed easiest to work with a primative dict rather than a more complex type. Maybe there is a way to pass complex types to rust via pyo3 though (do you happen to know?). Anyways, this internal decision shouldn't affect the public API of the python package, I agree.

I think option B works for me.

I wonder about the naming - is grimp.Level meaningful enough? Or should it be something more verbose but explicit e.g. grimp.LayeredArchitectureLevel? What do you think?

Should this Level type (or whatever we decide to name it) be a ValueObject https://github.com/seddonym/grimp/blob/master/src/grimp/domain/valueobjects.py?

when calling the rust function it seemed easiest to work with a primative dict rather than a more complex type

Agreed. It's fine to convert the object to a dict in Python before passing to Rust, that's just an internal implementation detail that can be easily changed.

I think option B works for me.

Let's go for that then! 🥳 Though you do make a good point about naming, I think Level is probably fine but let me sleep on it. It's an easy change if we do need to rethink it before release.

Should this Level type (or whatever we decide to name it) be a ValueObject

Yes please!

Updated in the option B fashion 🤞

Rather than a tuple of sets accept a tuple of dicts. This dict will contain the level layers as well as an boolean indicating whether the layers within the level should be independent of each other.

Peter554 · 2023-12-14T18:17:48Z

docs/usage.rst


-      .. attribute:: importer


There were some indentation issues here, which got fixed here too.

Before

After

seddonym

Really good stuff! Thanks for such a high quality pull request.

seddonym · 2023-12-15T14:11:48Z

src/grimp/application/ports/graph.py

@@ -284,13 +284,15 @@ def find_illegal_dependencies_for_layers(

        Additionally, multiple layers can be grouped together at the same level; for example
        `mypackage.utils` and `mypackage.logging` might sit at the bottom, so they cannot
-        import from any other layers. Layers at the same level must be independent, so any
-        dependencies in either direction will be treated as illegal.
+        import from any other layers. When a simple set of sibling layers is passed then they


Would slightly prefer the documentation (at least in this docstring) to indicate Level as being the primary type that is passed, and then mention the single-string / set of strings as conveniences that will be converted to levels. But not a blocker.

Peter554 force-pushed the allow-layer-sibling-independence-to-be-configured branch 4 times, most recently from ece664b to bfd8ba2 Compare November 21, 2023 16:31

Peter554 mentioned this pull request Nov 21, 2023

Proposal: Allow configuring sibling independence seddonym/import-linter#208

Closed

Peter554 force-pushed the allow-layer-sibling-independence-to-be-configured branch 5 times, most recently from 8ddde4a to 3f72ae4 Compare November 28, 2023 20:15

Peter554 commented Nov 28, 2023

View reviewed changes

rust/src/lib.rs Outdated Show resolved Hide resolved

src/grimp/adaptors/_layers.py Show resolved Hide resolved

Peter554 marked this pull request as ready for review November 28, 2023 20:32

seddonym closed this Nov 29, 2023

seddonym reopened this Nov 30, 2023

seddonym requested changes Dec 13, 2023

View reviewed changes

Peter554 force-pushed the allow-layer-sibling-independence-to-be-configured branch 2 times, most recently from f1b008a to 7eab8c4 Compare December 14, 2023 17:51

Peter554 added 4 commits December 14, 2023 18:52

Adapt rust find_illegal_dependencies to accept tuple of level dicts

d3e6003

Rather than a tuple of sets accept a tuple of dicts. This dict will contain the level layers as well as an boolean indicating whether the layers within the level should be independent of each other.

Add Level object to describe a level within a layered architecture

1bbc59a

Make Level the primary API of find_illegal_dependencies_for_layers

8213a3f

Extend test test_direct_illegal_between_sibling_layers

56364a0

Peter554 force-pushed the allow-layer-sibling-independence-to-be-configured branch from 7eab8c4 to 0ba304b Compare December 14, 2023 17:52

Peter554 added 4 commits December 14, 2023 19:08

Update documentation

64b47c9

Update CHANGELOG

d82114a

Update AUTHORS

221ae97

Remove dead code in _layers.py

155450d

Peter554 force-pushed the allow-layer-sibling-independence-to-be-configured branch from 0ba304b to 155450d Compare December 14, 2023 18:08

Peter554 commented Dec 14, 2023

View reviewed changes

Peter554 requested a review from seddonym December 14, 2023 18:18

seddonym approved these changes Dec 15, 2023

View reviewed changes

seddonym merged commit 88d4912 into seddonym:master Dec 15, 2023
12 checks passed

Peter554 mentioned this pull request Dec 15, 2023

Add support for non-independent sibling modules in layer contracts seddonym/import-linter#209

Merged

Peter554 deleted the allow-layer-sibling-independence-to-be-configured branch January 3, 2024 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow layer sibling independence to be configured #136

Allow layer sibling independence to be configured #136

Peter554 commented Nov 21, 2023 •

edited

Loading

Peter554 commented Nov 28, 2023

seddonym commented Nov 29, 2023

Peter554 commented Nov 29, 2023

seddonym commented Nov 29, 2023

seddonym left a comment

seddonym Dec 13, 2023

Peter554 Dec 14, 2023

seddonym Dec 13, 2023

Peter554 Dec 14, 2023

seddonym Dec 13, 2023

Peter554 Dec 14, 2023

seddonym Dec 13, 2023

Peter554 Dec 14, 2023

Peter554 Dec 14, 2023

seddonym Dec 14, 2023

seddonym Dec 14, 2023

Peter554 Dec 14, 2023

Peter554 Dec 14, 2023

seddonym left a comment

seddonym Dec 15, 2023

		@@ -15,6 +15,11 @@ class DetailedImport(TypedDict):
		line_contents: str


		class Level(TypedDict):

Allow layer sibling independence to be configured #136

Allow layer sibling independence to be configured #136

Conversation

Peter554 commented Nov 21, 2023 • edited Loading

Peter554 commented Nov 28, 2023

seddonym commented Nov 29, 2023

Peter554 commented Nov 29, 2023

seddonym commented Nov 29, 2023

seddonym left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seddonym left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peter554 commented Nov 21, 2023 •

edited

Loading