-
Notifications
You must be signed in to change notification settings - Fork 52
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
Allow to ignore a list of RegEx expressions in layer contract #46
base: master
Are you sure you want to change the base?
Conversation
Thanks so much for this, looking forward to reviewing it. Before I merge it I'll want to make sure there are tests - if you have time to do that, go ahead! Otherwise I'll add them when I get the chance. |
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.
Thanks for the great PR!
This has helped clarify my thinking about how to approach this feature. To begin with at least, I think it would be better to implement this as a top-level configuration, rather than on a contract-by-contract basis.
The main reason for this is that because the layers contract uses find_shortest_chain
to analyse the graph. Because that includes indirect imports, the excluded modules might still trigger a contract failure.
I think the better approach is to remove the ignored modules from the graph altogether before analysing anything. This is easier to do globally across the whole run, as it means we don't need to add the modules back afterwards.
To help with this, I've today released a new version of Grimp that has a remove_module method (v1.0b11).
So, building on your PR, here's what I think we should do:
- A top level configuration called
exclude_modules
which takes one or more modules that should be removed from the graph after it is built. - The setting should support pattern matching, though personally I feel it would be better just to use
*
wildcards but not support regular expressions, as they can be difficult to read and we probably don't need that kind of power here. I'm open to persuasion though! The fnmatch might be good for that. - The main implementation code would be in
importlinter.adapters.building.GraphBuilder
, which I imagine would remove any matching modules after the graph is built, usinggraph.remove_module
. - GraphBuilder would need to support a new
exclude_modules
parameter, to see how to get this from the top level configuration take a look at howinclude_external_packages
is handled. - We'd need to include documentation for the
exclude_modules
. - We'd need tests.
Let me know what you think of that approach, and also whether or not you'd like to contribute any code towards it. If not, I'll list you as a contributor anyway for your help so far.
Thanks again - and let me know if you have any questions.
I don't know if i will have much time in the near future but will try my best to do some advances. |
Cool, stay in touch. If I end up implementing it I'll let you know too. |
Fixes #44