Improve behaviour of option -W #933
Conversation
…now makes pydoctor exit with code 3. As a drive-by-change stop emitting warnings when parsing a regex. Fixes #930
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #933 +/- ##
==========================================
+ Coverage 93.26% 93.28% +0.01%
==========================================
Files 48 48
Lines 8895 8906 +11
Branches 1634 1634
==========================================
+ Hits 8296 8308 +12
Misses 339 339
+ Partials 260 259 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…no_such_option_exits_code3
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
twm
left a comment
twm
left a comment
There was a problem hiding this comment.
Looks good! Please do fix the changelog message before merge.
Thanks!
| from pydoctor.utils import warn | ||
| warn(f"Your custom template '{template.name}' is out of date, " | ||
| "information might be missing. " | ||
| "Latest templates are available to download from our github." ) |
There was a problem hiding this comment.
FWIW, a link would be more actionable.
There was a problem hiding this comment.
Yes I agree, but the chalenge is to provide the right link. And that depends on the current pydoctor version, so this would add more complexity
| """ | ||
| import warnings | ||
| warnings.warn(msg, category=PydoctorWarning) | ||
| _warned.set(True) |
There was a problem hiding this comment.
It feels a little delicate that all warnings emitted by pydoctor need to go through this function for the error status to track. There's nothing beyond familiarity with the codebase to guide future contributions down this path.
Could this be made more robust? Perhaps a lint rule could forbid direct use of warnings.warn()?
There was a problem hiding this comment.
We don’t have this kind of custom linting in place at the moment. I suppose this can be implemented with a semgrep pattern. But do you have another idea to enforce this ?
There was a problem hiding this comment.
Perhaps flake8-tidy-imports banned-api? I've used it to banish os.system() before.
Co-authored-by: Tom Most <[email protected]>
|
Many thanks for your review @twm, I’ll fix the CI, create issues for improvements cited and merge the PR. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
According to pydoctor_primer, this change doesn't affect pydoctor warnings on a corpus of open source code. ✅ |
2 participants
When option -W is used, all warnings issued with the warnings module now makes pydoctor exit with code 3.
As a drive-by-change stop emitting warnings when parsing a regex.
Fixes #930