Module wildcard expression in independence contract #220
Conversation
sbrugman
changed the title
sbrugman
force-pushed
the
module-wildcard-expression
branch
from
7cd96f7
to
29e0a86
Compare
There was a problem hiding this comment.
Thanks for looking into this!
The main thing I think we need to address before this is mergeable is that we should distinguish between modules and module expressions in our modelling.
We also need test coverage. I would expect this to include:
- Unit tests of the proposed module expressions to modules function in
contract_utils. - Test cases in
test_independence.
Of course docs need to be added - it would be helpful if you did a first draft but I don't mind doing that if you prefer.
Finally, would you mind adding something to CHANGELOG.rst and your name to AUTHORS.rst.
Feel free to reach out if you want to discuss anything and thanks again for taking the time to look at this.
| @@ -27,6 +27,9 @@ def __init__(self, name: str) -> None: | |||
| """ | |||
| self.name = name | |||
|
|
|||
| def has_wildcard_expression(self) -> bool: | |||
There was a problem hiding this comment.
My concern with this approach is that we're conflating modules with module expressions. I think it's important to keep this distinction, as we have done with the difference between the DirectImport and ImportExpression types.
With that in mind, I think we should add a new type called a ModuleExpression together with a ModuleExpressionField. We should then adjust the independence contract module field to be like so:
class IndependenceContract(Contract):
modules = fields.ListField(subfield=fields.ModuleExpressionField())
...
The IndependenceContract.check method can then call a function in contract_utils (maybe called resolve_module_expressions?) which would turn an iterable of ModuleExpression objects into a set of Module objects. What do you think of that approach?
| @@ -7,6 +7,21 @@ | |||
| FieldValue = TypeVar("FieldValue") | |||
|
|
|||
|
|
|||
| def _validate_wildcard(expression: str) -> None: | |||
There was a problem hiding this comment.
I wonder if we should be careful about using the same wildcard syntax used by ignored imports. For example, what would it actually mean if we listed mypackage.** in an independence contract? What about mypackage.*.foo?
I had been anticipating a much more limited set of expressions: i.e. just of the form mypackage.*.
Then again, if the functionality is already there, then perhaps it's counterproductive to special-case it. People may come up with use cases we hadn't thought of. What do you think?
| @@ -1,6 +1,8 @@ | |||
| from __future__ import annotations | |||
| @@ -181,25 +198,11 @@ def parse(self, raw_data: Union[str, List]) -> ImportExpression: | |||
| if not (importer and imported): | |||
| raise ValidationError('Must be in the form "package.importer -> package.imported".') | |||
|
|
|||
| self._validate_wildcard(importer) | |||
| self._validate_wildcard(imported) | |||
| _validate_wildcard(importer) | |||
There was a problem hiding this comment.
Not a blocker, but if we introduce the concept of ModuleExpression, it might be nice to use them within ImportExpression objects, which could become a pair of ModuleExpressions.
Reuses the existing wildcard logic to resolve wildcards in module names in the independence contract
Closes #56