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

Exclude modules from the graph, according to wildcard expressions #44

Closed
iagocanalejas opened this issue May 15, 2019 · 16 comments · Fixed by #103 · May be fixed by #46
Closed

Exclude modules from the graph, according to wildcard expressions #44

iagocanalejas opened this issue May 15, 2019 · 16 comments · Fixed by #103 · May be fixed by #46

Comments

@iagocanalejas
Copy link

Allow ignored regex expressions in layers contract.

As a example y don't want my *.tests.* modules to be processed.

@seddonym seddonym changed the title Ignore regex Support wildcards in ignore_imports May 16, 2019
@seddonym seddonym changed the title Support wildcards in ignore_imports Exclude modules from the graph, according to wildcard expressions May 16, 2019
@seddonym
Copy link
Owner

Hi Iago, thanks for the feature request.

A good way to go with this, I think, is to have a configuration option that allows modules to be ignored (currently you can only ignore relationships between modules, using theignore_imports option). Wildcard support would be a good idea as part of this.

I think this is potentially a good idea, I can't give you a date by which I'd implement it though. I'd certainly consider a pull request for it.

In the meantime you could implement this feature using a custom contract type. The approach would be to subclass importlinter.contracts.LayersContract contract and then override the check method to remove the tests modules from the graph before running the contract check. Unfortunately Grimp doesn't yet currently provide a .remove_import method on the graph, but the following hack should work:

graph._networkx_graph.remove_node("mypackage.tests.foo")

@iagocanalejas
Copy link
Author

Thanks for quick answer, i will try your solution and i also will try to have some time to make a PR adding wildcard support.

@seddonym
Copy link
Owner

Great - let me know if you need any pointers.

@iagocanalejas
Copy link
Author

Can't make it work with a custom contract. Even if i just subclass LayersContract using a custom contract just returns 'ListField' object is not iterable

[importlinter]
root_package = apps
contract_types =
    custom_layer: docs.layers.CustomLayersContract
class CustomLayersContract(LayersContract):
    pass

Results in:

'ListField' object is not iterable

@seddonym
Copy link
Owner

You've discovered a bug! I've been able to reproduce this and have added it as an issue here: #45

In the meantime, the workaround is just to redeclare the fields explicitly in your custom contract, i.e.

class CustomLayersContract(LayersContract):
    containers = LayersContract.containers
    layers = LayersContract.layers
    ignore_imports = LayersContract.ignore_imports

@kasium
Copy link
Contributor

kasium commented Aug 4, 2021

@seddonym @iagocanalejas this issue and the PR seem a little bit outdated 😄

I would like to extend this feature request to the independence type. Basically the ignore option should allow wildcards. If you like, I can give it a try building on top of the current PR. This would also help a project of mine where else we need to define a long list of ignores due the extensive usage of private submodules

@seddonym
Copy link
Owner

seddonym commented Aug 5, 2021

Hi @kasium - thanks for reaching out. I'd be happy to review a PR from you if you have capacity to move this forward - might be easier to start a dedicated PR though.

@kasium
Copy link
Contributor

kasium commented Aug 5, 2021

@seddonym before I start a simple question: What kind of wildcards do we want to support?

  • full blown regex (how can we decide if the module name is in fact a regex)
  • simple wildcards (*, ?)

I would go for the second one since both are invalid identifier names in python

@seddonym
Copy link
Owner

seddonym commented Aug 5, 2021

Simple wildcards I think. See https://docs.python.org/3/library/fnmatch.html. Have a look too at my comment on the other PR.

@kasium
Copy link
Contributor

kasium commented Aug 5, 2021

I see that make sense. So for which fields to you see this feature? Personally, I only have demand to have it for ignores and there only on the left side.

Implementation wise, I currently think of a special class WildcardModule or so. Only supporting it for ignores would make it more easy to add, because it would only affect the pop_imports method

@seddonym
Copy link
Owner

seddonym commented Aug 5, 2021

So for which fields to you see this feature?

Let's stick to ignore_imports for the time being. Since we now make a copy of the graph for each contract, it's okay to remove modules before analysing it.

Personally, I only have demand to have it for ignores and there only on the left side.

It's probably simpler for it to be on both sides, so we have something like this?

ignore_imports:
    *.tests.* -> myapp.utils.*

Implementation wise, I currently think of a special class WildcardModule or so.

I reckon probably the best direction is to adjust the DirectImportField and pop_imports to support wildcards in the imports, but possibly there's a better option.

Let me know if you want to chat further before getting stuck in.

@kasium
Copy link
Contributor

kasium commented Aug 6, 2021

Thanks a lot for your support. I tried to get some more information and ideas, and here are my (hopefully) final suggestion:

I guess that pop_import is the wrong place to handle the wildcards, because DirectImportField generates a DirectImport which again uses a Module. All of them have methods which are not designed to handle wildcards.
Therefore I think that improving the parsing of DirectImportField is the easiest way.

  1. Check if the module name contains wildcards, if not, proceed as normal
  2. Check if the first part of the module contains a wildcard. If yes, raise an error (this is important for the next steps)
  3. Allow the parse method to receive the graph object
  4. Get all submodules of the first module (therefore it can't handle a wildcard). Then check for each child if it's matched
  5. Return a list of DirectImport instead of only one object. This would require some more logic in Contract._populate_fields but not too much

So in short, instead of letting the user write down any combination, we generate it for him/her.

This idea feels to be more elegant and doesn't require changes in grimp. Also it leaves code at one place and all other objects are still the same.

What do you think about this?

@seddonym
Copy link
Owner

seddonym commented Aug 6, 2021

I think the simplest approach would be to allow wildcards in DirectImports (which already is the case) add an optional explode_wildcards argument to the pop_imports helper.

The contracts implementing ignore_imports would not need to change, other than explicitly passing in that argument:

class IndependenceContract(Contract):
    ...
    ignore_imports = fields.SetField(subfield=fields.DirectImportField(), required=False)

    def check(self, graph: ImportGraph) -> ContractCheck:
        ...
        helpers.pop_imports(
            graph, self.ignore_imports if self.ignore_imports else []  # type: ignore,
            explode_wildcards=True,
        )

helpers.pop_imports then becomes:

def pop_imports(
    graph: ImportGraph, imports: Iterable[DirectImport], explode_wildcards: bool = False
) -> List[Dict[str, Union[str, int]]]:
    """
    Removes the supplied direct imports from the graph.

    Args:
        ...
        explode_wildcards: If True, any DirectImports containing `*`s will be converted
                           into every import present in the graph that fits that pattern.
                           At least one import must match each wildcarded import, otherwise
                           MissingImport will be raised.
    Returns:
        The list of import details that were removed, including any additional metadata.

    Raises:
        MissingImport if the import is not present in the graph.
    """

Check if the first part of the module contains a wildcard. If yes, raise an error

I don't see why we can't support 'root' wildcards. The graph can support multiple root packages (they will have been passed in as root_package_names).

How does that sound?

@kasium
Copy link
Contributor

kasium commented Aug 6, 2021

Thanks for the suggestion.

My worry about moving the "explosion" to pop_imports is, that the methods in Module like parent or is_child_of need to be able to handle wildcards as well. Or do I miss something?

Also in grimp I couldn't find a method to get all root modules, therefore the limitation of the root package. But I guess I need to use the root packages provided to the config

@seddonym
Copy link
Owner

seddonym commented Aug 6, 2021

My worry about moving the "explosion" to pop_imports is, that the methods in Module like parent or is_child_of need to be able to handle wildcards as well.

Hmm, you make a good point. I don't think Module objects should be doing that kind of thing as they'd need knowledge of the graph.

The challenge is that up until this point, DirectImport and Module have been used to represent two things: actual direct imports and modules, but also user input in the contract.

Perhaps it would make more sense to introduce a new class similar to DirectImport but which represents just what the user passed in. Not sure what the best is but "ImportExpression" is the first thing that comes to mind. We could then replace DirectImportField with ImportExpressionField which returns an ImportExpression consisting merely of an importer string and an imported string (not Modules).

We could leave pop_imports as it is but introduce a new function which converts the import expressions (which might have wildcards in) to direct imports:

def explode_import_expressions(graph: ImportGraph, imports: Iterable[ImportExpression]) -> Iterable[DirectImport]:
   ...

Any better?

@kasium
Copy link
Contributor

kasium commented Aug 7, 2021

Let me give it a try after the weekend. I like the idea

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 a pull request may close this issue.

3 participants