-
Notifications
You must be signed in to change notification settings - Fork 7
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
ADR on import structure #8500
Open
wouterdb
wants to merge
5
commits into
master
Choose a base branch
from
issue/adr_on_import_structure
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
ADR on import structure #8500
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
a94e0b1
proposal
wouterdb 62bb3fa
Merge remote-tracking branch 'origin/master' into issue/adr_on_import…
wouterdb 5901c75
Merge remote-tracking branch 'origin/master' into issue/adr_on_import…
wouterdb 3219f98
update package names
wouterdb dfd598b
Update docs/adr/0004-import-structure.md
wouterdb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,94 @@ | ||||||||||||||||||||||||||||||
# Intended module structure to prevent import loops | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
* Status: proposed <!-- optional | rejected | accepted | deprecated | ... | superseded by [ADR-0000](0000-logging-warnings-using-the-python-warnings-or-logging-module.md)] --> | ||||||||||||||||||||||||||||||
* Deciders: [wouter, ] <!-- optional --> | ||||||||||||||||||||||||||||||
* Date: 19/12/2024 | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Technical Story: https://github.com/inmanta/inmanta-core/pull/8466 | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
## Context and Problem Statement | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Our import have become quite tangled, where every import imports most of the rest of the code. | ||||||||||||||||||||||||||||||
This is inefficient, it leads to import loops and it makes the code hard to reason about. | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
## Considered Options | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
1. General import rules 1 | ||||||||||||||||||||||||||||||
2. [option 2] | ||||||||||||||||||||||||||||||
3. [option 3] | ||||||||||||||||||||||||||||||
* ... <!-- numbers of options can vary --> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
## Decision Outcome | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Chosen option: "[option 1]", because [justification. e.g., only option, which meets k.o. criterion decision driver | which resolves force force | ... | comes out best (see below)]. | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
### Positive Consequences <!-- optional --> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
* [e.g., improvement of quality attribute satisfaction, follow-up decisions required, ...] | ||||||||||||||||||||||||||||||
* ... | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
### Negative Consequences <!-- optional --> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
* [e.g., compromising quality attribute, follow-up decisions required, ...] | ||||||||||||||||||||||||||||||
* ... | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
## Pros and Cons of the Options <!-- optional --> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
### General import rules 1 | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
- a module should have a clear responsibility, | ||||||||||||||||||||||||||||||
- which should be documented | ||||||||||||||||||||||||||||||
- from that responsibility can be derived its knowledge about the world around it, | ||||||||||||||||||||||||||||||
- which in turn drives which other modules it "sees" | ||||||||||||||||||||||||||||||
- keep `__init__` light, as it is always imported when a submodule is imported. | ||||||||||||||||||||||||||||||
- a package can offer an interface module (called now something like `model`/`types`/`__init__`)..... | ||||||||||||||||||||||||||||||
- The interface module contains the external interface of the module for other module to consume | ||||||||||||||||||||||||||||||
- superclasses | ||||||||||||||||||||||||||||||
- exceptions | ||||||||||||||||||||||||||||||
- interface types (executor.executor e.g), | ||||||||||||||||||||||||||||||
- datatypes,... | ||||||||||||||||||||||||||||||
- The interface module should import the least stuff possible | ||||||||||||||||||||||||||||||
- It should never import any non-interface modules | ||||||||||||||||||||||||||||||
- typing only imports can do what they want | ||||||||||||||||||||||||||||||
- Packages should roughly form a tree, | ||||||||||||||||||||||||||||||
- where only composing modules import the non-interface modules of composed packages. | ||||||||||||||||||||||||||||||
- composing interface modules should also only import interface modules of composed modules | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
e.g. consider `inmanta.agent.executor.executor` and `inmanta.deploy` packages and the new_agent | ||||||||||||||||||||||||||||||
- `inmanta.agent.executor.executor` is the interface package for the executor framework | ||||||||||||||||||||||||||||||
- `inmanta.deploy` is the package containing the scheduler (it has no external interface package) | ||||||||||||||||||||||||||||||
- `inmanta.deploy` imports `inmanta.agent.executor.executor` | ||||||||||||||||||||||||||||||
- `inmanta.agent.agent_new` imports both the interface and concrete implementations of both executor and deploy (top level) | ||||||||||||||||||||||||||||||
- `inmanta.deploy` doesn't import the non-interface parts of `inmanta.agent.executor.executor` because it never constructs executors (it is constructed with a reference to a executor manager, which serves as a factory) | ||||||||||||||||||||||||||||||
- `inmanta.agent.executor.executor` doesn't know about deploy at all | ||||||||||||||||||||||||||||||
Comment on lines
+60
to
+66
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.
Suggested change
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
* Good, because it offers guidance | ||||||||||||||||||||||||||||||
* Good, because reduces loops | ||||||||||||||||||||||||||||||
* Bad, because not super exact | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
### [option 2] | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
[example | description | pointer to more information | ...] <!-- optional --> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
* Good, because [argument a] | ||||||||||||||||||||||||||||||
* Good, because [argument b] | ||||||||||||||||||||||||||||||
* Bad, because [argument c] | ||||||||||||||||||||||||||||||
* ... <!-- numbers of pros and cons can vary --> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
### [option 3] | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
[example | description | pointer to more information | ...] <!-- optional --> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
* Good, because [argument a] | ||||||||||||||||||||||||||||||
* Good, because [argument b] | ||||||||||||||||||||||||||||||
* Bad, because [argument c] | ||||||||||||||||||||||||||||||
* ... <!-- numbers of pros and cons can vary --> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
## Links <!-- optional --> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
* [Link type] [Link to ADR] <!-- example: Refined by [ADR-0000](0000-logging-warnings-using-the-python-warnings-or-logging-module.md) --> | ||||||||||||||||||||||||||||||
* ... <!-- numbers of links can vary --> |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add naming guidelines for such interface modules ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
<package_name>_interface
or<package_name>_api
?And do we want to encourage using the
__init__.py
file more for this or does it contradict with the rulekeep __init__ light
?Is this more a case by case thing where the interface can live in the
__init__
if it's not too heavy but it should live in its own file as it grows ?