Skip to content
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.

Sign up for GitHub

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
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

ADR on import structure #8500

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a94e0b1
proposal
wouterdb Dec 19, 2024
62bb3fa
Merge remote-tracking branch 'origin/master' into issue/adr_on_import…
wouterdb Dec 19, 2024
5901c75
Merge remote-tracking branch 'origin/master' into issue/adr_on_import…
wouterdb Feb 20, 2025
3219f98
update package names
wouterdb Feb 20, 2025
dfd598b
Update docs/adr/0004-import-structure.md
wouterdb Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions docs/adr/0004-import-structure.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ Chosen option: "[option 1]", because [justification. e.g., only option, which me
- 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.
- 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__`).....
Copy link
Contributor

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have suggestions?

Copy link
Contributor

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 rule keep __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 ?

- The interface module contains the external interface of the module for other module to consume
- superclasses
Expand All @@ -57,12 +57,13 @@ Chosen option: "[option 1]", because [justification. e.g., only option, which me
- composing interface modules should also only import interface modules of composed modules


e.g. consider executor and deploy packages and the new_agent
- executor.executor is the interface package
- deploy imports executor.executor
- new_agent import both the interface and concrete implementations of both executor and deploy (top level)
- deploy doesn't import the non-interface parts of executor because it never constructs executors (it is constructed with a reference to a executor manager, which serves as a factory)
- executor doesn't know about deploy at all
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
e.g. consider `inmanta.agent.executor` and `inmanta.deploy` packages and the new_agent
- `inmanta.agent.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.scheduler` imports `inmanta.agent.executor`
- `inmanta.agent.agent_new` imports both the interface and concrete implementations of both executor and deploy (top level)
- `inmanta.deploy.scheduler` doesn't import the non-interface parts of `inmanta.agent.executor` because it never constructs executors (They are constructed with a reference to a executor manager, which serves as a factory)
- `inmanta.agent.executor` doesn't know about deploy at all



* Good, because it offers guidance
Expand Down