infra: add griffe to public-symbols check CI #4633
Conversation
Signed-off-by: emdneto <[email protected]>
emdneto
added
build & infra
| commands = | ||
| ; griffe check before to fail fast if there are any issues | ||
| {toxinidir}/scripts/griffe_check.sh | ||
| python {toxinidir}/scripts/public_symbols_checker.py |
There was a problem hiding this comment.
Will this catch anything than griffe won't?
There was a problem hiding this comment.
I don't know, to be honest. I left it here to keep what we have now 😅
There was a problem hiding this comment.
Yeah, we can run both for a release and then drop the custom one if it doesn't raise anything.
There was a problem hiding this comment.
@xrmx I think the main difference is that the public_symbols_checker.py script will check for additions. Griffe will be checking for removed/moved symbols
There was a problem hiding this comment.
I think griffe check misses a few other important cases we have related to modifying base classes
- Based on my testing, it doesn't detect a new parameter added to a method which makes older signatures incompatible. See feature: API checks: Check for types compatibility mkdocstrings/griffe#113
- Haven't tested but I don't think it will detect adding a new mandatory method to an ABC, but I might be wrong. Remember v1.23.0 added an abstract method to Meter, breaking subclasses #3710
| local package_name | ||
| local search_path="" | ||
|
|
||
| if [[ "$package_spec" == *"/"* ]]; then |
There was a problem hiding this comment.
sorry my bash isn't that great. when tox calls with no arguments, will it check all directories? This might get annoying for test directories.
There was a problem hiding this comment.
Yes, tests will also be checked 😓. I agree it will be annoying. Let me see if we can exclude them.
There was a problem hiding this comment.
thanks for the approval, but I'll put it in draft to write the check using griffe Python API. It will be easier to maintain.
Close #4619