GeezerVison is showing github.com — Deprecate suspicious.py? · Issue #86404 · python/cpython
Deprecate suspicious.py? #86404Description
| BPO |
42238 |
| Nosy |
@birkenfeld, @rhettinger, @ned-deily, @ambv, @willingc, @JulienPalard, @pablogsal, @erlend-aasland, @AlexWaygood |
| PRs |
bpo-42238: Doc: Remove make suspicious from the CI and docs builds. #23313bpo-42238: Check Misc/NEWS.d/next/ for reStructuredText issues. #23802bpo-42238: Doc CI: Disable suspicious checks. #26575bpo-42238: rstlint: Add two new checks. #26966bpo-42238: Fix small rst issue in NEWS.d/. #27238bpo-42238: [doc] Some more make suspicious false positives. #27945bpo-42238: [doc] remove unused, and deduplicate, suspicious ignore rules. #28137bpo-42238: [doc]: update susp-ignored.csv after a98b273c. #28827bpo-42238: [doc]: Hide false positive in make suspicious. #29636bpo-42238: [doc] Announce the future removal of make suspicous. #29652bpo-42238: [doc]: Spotted errors while working on rstlint. #30879bpo-42238: [doc] Avoid hardcoding fast-moving lines in susp-ignored.csv. #30981bpo-42238: [doc] moving from rstlint.py to sphinx-lint. #31097bpo-42238: [doc] Some more make suspicious false positives. #31977bpo-42238: [doc] Some lines moved in rst, but had hardcoded lineno in… #32070bpo-42238: [doc]: make suspicious: false positive. #32292bpo-42238: [doc]: A make sucpicious false positive. #32329 |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/JulienPalard'
closed_at = None
created_at = <Date 2020-11-01.23:49:23.937>
labels = ['docs']
title = 'Deprecate suspicious.py?'
updated_at = <Date 2022-04-06.11:53:50.624>
user = 'https://github.com/JulienPalard' bugs.python.org fields:
activity = <Date 2022-04-06.11:53:50.624>
actor = 'mdk'
assignee = 'mdk'
closed = False
closed_date = None
closer = None
components = ['Documentation']
creation = <Date 2020-11-01.23:49:23.937>
creator = 'mdk'
dependencies = []
files = []
hgrepos = []
issue_num = 42238
keywords = ['patch']
message_count = 56.0
messages = ['380167', '380169', '380171', '381075', '381802', '382219', '383149', '383171', '383292', '390898', '390904', '391163', '391164', '391177', '391182', '391185', '391209', '391212', '392165', '392177', '395311', '396567', '396568', '396774', '396891', '397792', '397802', '400259', '400324', '401123', '403523', '404067', '406575', '406604', '406654', '406810', '406978', '408724', '411565', '411571', '411636', '411642', '411983', '412135', '412136', '412355', '412377', '412382', '412403', '412980', '414984', '415860', '416639', '416643', '416759', '416861']
nosy_count = 9.0
nosy_names = ['georg.brandl', 'rhettinger', 'ned.deily', 'lukasz.langa', 'willingc', 'mdk', 'pablogsal', 'erlendaasland', 'AlexWaygood']
pr_nums = ['23313', '23802', '26575', '26966', '27238', '27945', '28137', '28827', '29636', '29652', '30879', '30981', '31097', '31977', '32070', '32292', '32329']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue42238'
versions = [] ActivityJulienPalard commented on Nov 1, 2020 I was not here 21 years ago when it was introduced [1], but according to the commit message it was introduced to find leftover Latex mardown.
It tries to find 4 patterns in Sphinx node text (not in raw rst files):
::(?=[^=])| # two :: (but NOT ::=)
This one has ~100 false positive in susp-ignored.csv (pypi classifiers, slices, ipv6, ...)
:[a-zA-Z][a-zA-Z0-9]+| # :foo
This one has ~300 false positive in susp-ignored.csv (slices, C:\, ipv6, ...)
| # (seldom used by itself)
This one has ~20 false positive in susp-ignored.csv (mostly reStructuredText in code-blocks)
(?<!\.)\.\.[ \t]*\w+: # .. foo: (but NOT ... else:)
This one does not have false positives.
The script, on my laptop (with a core i9), is slow (4mn20s), and it's probably way slower on the CI.
I tried to search for suspicious is:pr in:comments on github to see if it's usefull:
Case where it actually helped:
I'd go for enhancing rstlint.py (which is fast, ~1s on my laptop) a bit to try to handle the `.. literalinclude:` missing a `:` errors, and dropping suspicious.
So I'd appreciate feedback on this script, did it helped you recently?
1: 700cf28 rhettinger commented on Nov 2, 2020 I would love to see this disappear. For me, it has been a recurring time waster. ned-deily commented on Nov 2, 2020 If it still does useful checks, we could just limit it to running by release managers during the release manufacturing process; it is already run then. It would still allow for problems to be caught and fixed by the RM prior to release tagging. But I don't have a strong opinion about its overall usefulness. I recall it catching at least one problem in a release I was involved with and that was probably before we had the CI checks. JulienPalard commented on Nov 16, 2020 Good idea Ned!
So proposed plan:
- Drop it from docs build and the CI to avoid time loss.
- Add it as a step of PEP-101, for a few release, for good measures
- I'll check it from time to time between releases, just to ensure it does not accumulate tons of things to do on the the release day.
If during a few release, we notice this tool is no loger usefull we'll be able to drop it.
If we spot some errors that can be migrated to the rstlint.py we'll do.
If this tool is in fact usefull, we'll have to think about it again. JulienPalard commented on Nov 25, 2020 JulienPalard commented on Dec 1, 2020 I'll try to track this closely, the drop of make suspicious has been merged in master, but not backported yet, one week ago.
Up to today, no commit has introduced suspicious warnings (0 false positive, 0 false negative).
I think I'll soon backport the PR, if no one objects, because the current situation is a bit risky: some PR could get merged to master but fail the tests during the backport. JulienPalard commented on Dec 16, 2020 Today I spotted one error that suspicious could have spotted:
-both ``Callable``s no longer validate their ``argtypes``, in
+both ``Callable``\ s no longer validate their ``argtypes``, in
I'm fixing it in #23800
make suspicious found it as:
[whatsnew/changelog:23] "`" found in "Callable``s no longer validate their ``argtypes"
So I'll doodle around this to see if tools/rstlint.py or Sphinx itself can see the error. JulienPalard commented on Dec 16, 2020 I created #23802 with a port of the "wrong backtick detector" (at least, one aspect from it) from suspicious to rstlint.
I dropped the suspicious check three weeks ago, and found a single committed issue since then, which was fixable (and is fixed) in rstlint, so I'll continue this experience and see where it goes. JulienPalard commented on Dec 18, 2020 vstinner commented on Apr 12, 2021 Pablo reenabled the check:
commit 20ac347 (upstream/master, master)
Author: Pablo Galindo <[email protected]>
Date: Mon Apr 12 20:53:15 2021 +0100
Fix Sphinx errors in the documentation and re-activate the suspicious check (GH-25368)
The suspicious check is still executed as part of the release process and release managers have been
lately fixing some actual errors that the suspicious target can find. For this reason, reactivate the suspicious
until we decide what to do in a coordinated fashion.
pablogsal commented on Apr 12, 2021 Background on why I reactivated it:
I every single alpha release I had to fix actual documentation errors (no false positives) like the one fixed here:
20ac347
I don't like the check, but the release management team don't like to have to restart the release process just fo fix a bunch of docs issues that should have been detected by the CI.
If we remove the fix, we need to coordinate all together because currently the status quo is that the RM team now is responsable of fixing the documentation errors as there are several release tools (and other ones) that depend on this target passing. rhettinger commented on Apr 16, 2021 If reactivated, the tool needs to be substantially improved. It is NOT smart. The false positives for slicing and logging examples are unnecessarily annoying. It creates a barrier for people submitting documentation patches. Each of the 367 entries in the susp-ignored file represents wasted time for contributors.
Also the CSV format is arcane, hard-to-read, and hard-to-edit. It looks like it was quickly thrown together by someone who didn't care about usability. Perhaps there should be a simpler tools that says, "take this current failure and mark it as a false positive" without trying to be over specific.
Mandatory checks with a high false positive rate are an anti-pattern for CI systems. Already we've had one case of a contributor (me) who abandoned a doc patch rather than fight this tooling. pablogsal commented on Apr 16, 2021 I agree with all you said, but I am against forcing the release management team to absorb the responsibility of fixing all the errors that the CI didn't catch, real or not. Releasing is already a multi-hour process and having many docs errors and formating mistakes to fix is a toll that should be paid at release time. added 3 commits that reference this issue on Oct 11, 2022This issue will close once commit 35438b7 is merged into the 'main' branch.35438b7 This issue will close once commit 64192cb is merged into the 'main' branch.64192cb This issue will close once commit dc5cf61 is merged into the 'main' branch.dc5cf61 vstinner commented on Oct 11, 2022 Reacted by Alex Waygood, Hugo van Kemenade and Ezio Melotti Reacted by Julien Palard
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|