Support wildcards in module ignores #103
Conversation
|
@seddonym here is the first implementation draft. Please take a look if you see any issues before I move to the tests |
Co-authored-by: David Seddon <[email protected]>
Co-authored-by: David Seddon <[email protected]>
|
So, any further comments? |
Sorry @kasium I thought I was waiting on you! I will try to look as soon as I can but it might be a week or so, sorry about that 😄 |
There was a problem hiding this comment.
This is shaping up really nicely.
Apart from a few tweaks, the main thing we're missing is tests. I suggest we stop marking this PR as a draft and move into the final stages!
PS I think you might need to rebase off the latest master - I tried to build the docs and there were some errors which aren't happening on master.
| graph: ImportGraph, expressions: Iterable[ImportExpression] | ||
| ) -> List[Dict[str, Union[str, int]]]: | ||
| imports: List[DirectImport] = [] | ||
| for expression in expressions: |
There was a problem hiding this comment.
Nice. I think it would be worth extracting lines 41 - 53 into a separate import_expressions_to_imports function, which would be a side-effect free function so easier to test.
It would be great to have unit test coverage too. If I were you I would focus the unit testing on the pure function import_expressions_to_imports - possibly don't need one for pop_import_expressions if we have coverage on the contract types instead.
There was a problem hiding this comment.
Added some basic tests
|
|
||
| def __init__(self, importer: str, imported: str) -> None: | ||
| self.importer = importer | ||
| self.imported = imported |
There was a problem hiding this comment.
Perhaps we should raise a ValueError if we get a malformed import expression, what do you think?
There was a problem hiding this comment.
I guess, then it make sense to extract the import expression validationto a util function or NOT to validate in ImportExpressionField
|
|
||
| The following options can be used in all contracts. | ||
|
|
||
| - ``ignore_imports``: Optional list of imports, each in the form ``mypackage.foo.importer -> mypackage.bar.imported``. |
There was a problem hiding this comment.
(Not related to documentation, but had to write this somewhere!) It would be great to have the following test coverage:
- Unit tests for wildcarded ignore_imports for each contract type (see tests/unit/contracts).
- Functional test coverage in tests/functional/test_lint_imports.py. I suggest just adding an illegal import, together with a wildcarded
ignore_importsthat covers it, to the contract insetup.cfgin that folder (which is used for checking contract adherence).
There was a problem hiding this comment.
@seddonym I can work further on the pure functions but I guess I don't have the time for extended tests. I really see the point of them, but it consumes a lot of time. Would you maybe just add them?
Already on latest master, no clue |
|
Thanks for the latest changes, they look great. I've now fixed the issue on master, so rebasing off that should hopefully get the tests passing. Let's get this out of draft and with the existing tests passing. I do want a bit more test coverage before merging to master but I'm happy to merge this into a branch on the main repo and then make the final changes there. Thanks again for the excellent work! |
Co-authored-by: David Seddon <[email protected]>
|
Great. Your feedback is always appreciated. I learned a lot during the iterations. |
Support wildcards in module ignores
Support wildcards in module ignores
Support wildcards in module ignores
Support wildcards in module ignores Co-authored-by: kasium <[email protected]>
Resolves #44