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.

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

Added support for extended interpolations for .ini files. #121

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nickswebsite
Copy link

I had a use case consisting of a rather complicated project with a lot of legacy code that we are trying to organize. Both the layers and independence contracts would have to have more ignores that would be practical. It seemed to be easier to define groups of modules explicitly (or ideally with wild cards) and enforce architecture that way.

This PR configures .ini files to use the configparser.ExtendedInterpolation(). Unfortunately, I don't think toml has built in support for interpolations, and I'm not really sure how each package manager will set up the ini parser when parsing setup.cfg, so I had no reliable way add the feature when handling those formats.

Tests pass

nickswebsite(.venv)$ tox

...

report run-test: commands[1] | coverage html
Wrote HTML report to htmlcov/index.html
_____________________________________________________________________________________________________________________ summary _____________________________________________________________________________________________________________________
  clean: commands succeeded
  check: commands succeeded
  docs: commands succeeded
  py37-notoml: commands succeeded
  py37-toml: commands succeeded
  py38-notoml: commands succeeded
  py38-toml: commands succeeded
  py39-notoml: commands succeeded
  py39-toml: commands succeeded
  py310-notoml: commands succeeded
  py310-toml: commands succeeded
  report: commands succeeded
  congratulations :)

Docs Build
Import-Linter-Docs-Screenshot

@seddonym
Copy link
Owner

Thanks for this - I hadn't come across INI file interpolation before. This seems like an elegant solution but I want to think about whether or not it adds unnecessary complexity.

It seemed to be easier to define groups of modules explicitly (or ideally with wild cards) and enforce architecture that way.

Are you aware that Import Linter does now have wildcard support for ignored imports? I'd be interested to know whether or not adding that to other fields (e.g. source_modules) would be preferable to the interpolation approach.

@nickswebsite
Copy link
Author

Yes, I saw that in the documentation, though it wasn't working for other configuration options. It would definitely be nice to define groups of modules using wild cards. A lot of projects developed by large teams could benefit from rules like:

type = forbidden
source_modules =
    *.models
    *.model
    *.datamodel
forbidden_modules =
    *.views
    *.view
    *.websocket
    *.websockets
    *.route_handers
    some_feature.*.views
    *.etc
    http

Having a wildcard that can extend to subpackages could also be beneficial. Without it, it is too easy to miss, say feature.models.submodels importing objects from other_feature.views.subviews.

@seddonym
Copy link
Owner

Having a wildcard that can extend to subpackages could also be beneficial.

Agreed. This is how forbidden contracts already behave - from the docs: "Descendants of each module will be checked - so if mypackage.one is forbidden from importing mypackage.two, then mypackage.one.blue will be forbidden from importing mypackage.two.green." Of course, we'd need to implement wildcard support first!

Regarding the contract you describe above, I do wonder if we're stretching the limits of the original intention of forbidden contracts, which are meant to be fairly simple. It might be worth considering viewing this as a layered architecture instead. That way you can use a layers contract, which allows you to list containers so you don't need to keep defining the containing packages:

[importlinter:contract:1]
name = My multiple package layers contract
type = layers
layers=
    (views)
    (view)
    (websocket)
    (websockets)
    (route_handlers)
    (models)
    (model)
    (datamodel)
containers=
    feature
    other_feature

Alternatively, you could consider creating directories to correspond to the two-tier architecture implied by that forbidden contract, i.e.

myfeature/
   data/
       models.py
       datamodel.py
   web/
       views.py
       websocket.py
       ...

Then the layers contract would simply be:

[importlinter:contract:1]
name = My multiple package layers contract
type = layers
layers=
    web
    data
containers=
    feature
    other_feature

@YehudaCorsia
Copy link

YehudaCorsia commented Oct 24, 2022

Just to mention that I would also like the ability to add groups
My team and I cannot use the layers approach
And we are trying to use the forbidden approach
but this is very difficult when you can't create groups of modules.

@seddonym
Copy link
Owner

And we are trying to use the forbidden approach
but this is very difficult when you can't create groups of modules.

Could you go into a bit more detail? Would be helpful to see some examples. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants