-
Notifications
You must be signed in to change notification settings - Fork 17
Closed layers #227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed layers #227
Changes from all commits
e0c2985
b5be3ff
026fc00
ac5b197
d096153
252fe62
7362cf9
2f33fee
5b669fb
10d41fb
5c6a9cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,9 @@ pub struct Level { | |
|
|
||
| #[getset(get_copy = "pub")] | ||
| independent: bool, | ||
|
|
||
| #[getset(get_copy = "pub")] | ||
| closed: bool, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, new, Getters)] | ||
|
|
@@ -57,7 +60,7 @@ impl Graph { | |
| .flat_map(|m| m.conv::<FxHashSet<_>>().with_descendants(self)) | ||
| .collect::<FxHashSet<_>>(); | ||
|
|
||
| self.generate_module_permutations(levels) | ||
| self.generate_illegal_import_permutations_for_layers(levels) | ||
| .into_par_iter() | ||
| .try_fold( | ||
| Vec::new, | ||
|
|
@@ -81,15 +84,20 @@ impl Graph { | |
| ) | ||
| } | ||
|
|
||
| fn generate_module_permutations(&self, levels: &[Level]) -> Vec<(ModuleToken, ModuleToken)> { | ||
| let mut permutations = vec![]; | ||
| /// Returns a set of tuples (importer, imported) describing the illegal | ||
| /// import permutations for the given layers. | ||
| fn generate_illegal_import_permutations_for_layers( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment in the docstring now. |
||
| &self, | ||
| levels: &[Level], | ||
| ) -> FxHashSet<(ModuleToken, ModuleToken)> { | ||
| let mut permutations = FxHashSet::default(); | ||
|
|
||
| for (index, level) in levels.iter().enumerate() { | ||
| for module in &level.layers { | ||
| // Should not be imported by lower layers. | ||
| for lower_level in &levels[index + 1..] { | ||
| for lower_module in &lower_level.layers { | ||
| permutations.push((*lower_module, *module)); | ||
| permutations.insert((*lower_module, *module)); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -99,8 +107,19 @@ impl Graph { | |
| if sibling_module == module { | ||
| continue; | ||
| } | ||
| permutations.push((*module, *sibling_module)); | ||
| permutations.insert((*module, *sibling_module)); | ||
| } | ||
| } | ||
|
|
||
| // Should not be imported by higher layers if there is a closed layer inbetween. | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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"?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Coming back to it, I agree with how you've done it here. Can you see any reason why we should allow indirect imports?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with you.
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). |
||
| let mut closed = false; | ||
Peter554 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for higher_level in levels[..index].iter().rev() { | ||
| if closed { | ||
| for higher_module in &higher_level.layers { | ||
| permutations.insert((*higher_module, *module)); | ||
| } | ||
| } | ||
| closed |= higher_level.closed; | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -208,3 +227,106 @@ impl Graph { | |
| ) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::graph::Graph; | ||
| use rustc_hash::FxHashSet; | ||
|
|
||
| #[test] | ||
| fn test_generate_module_permutations_simple_layers() { | ||
| let mut graph = Graph::default(); | ||
|
|
||
| let top_module = graph.get_or_add_module("app.top").token; | ||
| let middle_module = graph.get_or_add_module("app.middle").token; | ||
| let bottom_module = graph.get_or_add_module("app.bottom").token; | ||
|
|
||
| let mut top_layer = FxHashSet::default(); | ||
| top_layer.insert(top_module); | ||
|
|
||
| let mut middle_layer = FxHashSet::default(); | ||
| middle_layer.insert(middle_module); | ||
|
|
||
| let mut bottom_layer = FxHashSet::default(); | ||
| bottom_layer.insert(bottom_module); | ||
|
|
||
| let top_level = Level::new(top_layer, false, false); | ||
| let middle_level = Level::new(middle_layer, false, false); | ||
| let bottom_level = Level::new(bottom_layer, false, false); | ||
|
|
||
| let levels = vec![top_level, middle_level, bottom_level]; | ||
|
|
||
| let permutations = graph.generate_illegal_import_permutations_for_layers(&levels); | ||
|
|
||
| assert_eq!( | ||
| permutations, | ||
| FxHashSet::from_iter([ | ||
| (bottom_module, middle_module), | ||
| (bottom_module, top_module), | ||
| (middle_module, top_module), | ||
| ]) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_generate_module_permutations_independent_layer() { | ||
| let mut graph = Graph::default(); | ||
|
|
||
| let module_a = graph.get_or_add_module("app.independent.a").token; | ||
| let module_b = graph.get_or_add_module("app.independent.b").token; | ||
|
|
||
| let mut independent_layer = FxHashSet::default(); | ||
| independent_layer.insert(module_a); | ||
| independent_layer.insert(module_b); | ||
|
|
||
| let independent_level = Level::new(independent_layer, true, false); | ||
|
|
||
| let levels = vec![independent_level]; | ||
|
|
||
| let permutations = graph.generate_illegal_import_permutations_for_layers(&levels); | ||
|
|
||
| assert_eq!( | ||
| permutations, | ||
| FxHashSet::from_iter([(module_a, module_b), (module_b, module_a),]) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_generate_module_permutations_closed_layer() { | ||
| let mut graph = Graph::default(); | ||
|
|
||
| // Create three layers with the middle one closed | ||
| let top_module = graph.get_or_add_module("app.top").token; | ||
| let middle_module = graph.get_or_add_module("app.middle").token; | ||
| let bottom_module = graph.get_or_add_module("app.bottom").token; | ||
|
|
||
| let mut top_layer = FxHashSet::default(); | ||
| top_layer.insert(top_module); | ||
|
|
||
| let mut middle_layer = FxHashSet::default(); | ||
| middle_layer.insert(middle_module); | ||
|
|
||
| let mut bottom_layer = FxHashSet::default(); | ||
| bottom_layer.insert(bottom_module); | ||
|
|
||
| let top_level = Level::new(top_layer, false, false); | ||
| let middle_level = Level::new(middle_layer, false, true); // Closed layer | ||
| let bottom_level = Level::new(bottom_layer, false, false); | ||
|
|
||
| let levels = vec![top_level, middle_level, bottom_level]; | ||
|
|
||
| let permutations = graph.generate_illegal_import_permutations_for_layers(&levels); | ||
|
|
||
| assert_eq!( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I think this would be clearer as a single assertion on the result.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| permutations, | ||
| FxHashSet::from_iter([ | ||
| (bottom_module, middle_module), | ||
| (bottom_module, top_module), | ||
| (middle_module, top_module), | ||
| // Top should not import Bottom due to closed middle layer | ||
| (top_module, bottom_module), | ||
| ]) | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ fn test_large_graph_deep_layers() { | |
| .token() | ||
| .conv::<FxHashSet<_>>(), | ||
| true, | ||
| false, | ||
| ) | ||
| }) | ||
| .collect(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,12 +69,15 @@ class Layer: | |
|
|
||
| module_tails: Set[str] | ||
| independent: bool | ||
| closed: bool | ||
|
|
||
| # A custom `__init__` is needed since `module_tails` is a variadic argument. | ||
| def __init__(self, *module_tails: str, independent: bool = True) -> None: | ||
| def __init__(self, *module_tails: str, independent: bool = True, closed: bool = False) -> None: | ||
| # `object.__setattr__` is needed since the dataclass is frozen. | ||
| object.__setattr__(self, "module_tails", set(module_tails)) | ||
| object.__setattr__(self, "independent", independent) | ||
| object.__setattr__(self, "closed", closed) | ||
|
|
||
| def __str__(self) -> str: | ||
| return f"{self.module_tails}, independent={self.independent}" | ||
| module_tails = sorted(self.module_tails) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
| return f"{module_tails}, independent={self.independent}, closed={self.closed}" | ||
Uh oh!
There was an error while loading. Please reload this page.