-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-106948: Doc config ignores more standard C functions #107301
Conversation
Complete nitpick_ignore list in Doc/conf.py to ignore more standard C functions, variables, macros, and also Win32 API functions and macros. No longer ignore "__int" C type: it should not be used in the doc.
@erlend-aasland: In commit b447e19 (PR #107062), you decided to not add @serhiy-storchaka wrote that we should not ignore functions which are referenced only once (or twice): #107062 (comment) This PR goes against that. I wrote it before reading PR #107062. I updated nitpick_ignore by going through the whole list of Do you want me to count how many times a function is called to decide to ignore it or not? I skipped many functions which don't belong to the C library (libc), but readline or other library. I also skipped |
('c:type', 'va_list'), | ||
('c:type', 'wchar'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a typo, not a c:type. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I wasn't sure about this one, ok, I will remove it. First, I'm trying to land PR #107302 to reduce the number of warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
('c:type', 'wchar'), | |
('c:type', 'wchar_t'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that wchar_t
is already listed right below, so the change here should be just removing it:
('c:type', 'wchar'), |
Smaller list is easier to maintain. A PR which adds less names is simpler to review (and yet somehow But if you and @erlend-aasland are fine with a longer list, I have no objections. Did you checked every one of these names? |
I share the same view as Serhiy. IMO, it makes sense to keep only the most common functions (C stdlib stuff) in this list, and use the I won't block this PR, but I won't approve it either ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but I'm not a C expert like the other folks here, so while I gave the list a skim and didn't spot any super-obvious false negatives, I could be easily be missing some.
I do share Erlend's and Serhiy's concerns about going overboard with an overly lengthy list that may contain other undetected mistakes that may in turn silence potentially valid errors.
On the other hand, while I'm a stickler for correctness and explicitness, that's balanced with the desire to reduce the burden on doc authors having to manually silence warnings to satisfy the CI, which annoys contributors, consumes non-trivial nitpickiness budget, reduces warning SNR and builds a habit of automatically silencing them rather than carefully considering how to handle each of them individually.
For me, its a trade between the chance of a false negative vs. the accumulated author time and annoyance dealing with the false positives the additional names would have prevented (unless they are somehow going to change frequently, I don't see that much real-world ongoing maintenance cost keeping them in conf.py
).
Personally, I'd rather see a few false negatives and have the time saved silencing false positives spent fixing true positives properly (instead of just habitually silencing them)—as you've shown admirable care about so far in #107302 . @vstinner , if you have (or can) checked them over again fairly carefully, then it seems to me that the benefit of the latter would likely outweigh any impact of the former, so I'd be +0.5 on this.
('c:type', 'va_list'), | ||
('c:type', 'wchar'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that wchar_t
is already listed right below, so the change here should be just removing it:
('c:type', 'wchar'), |
Ok, I will try this approach. I started with PR #107329 which uses a few |
Complete nitpick_ignore list in Doc/conf.py to ignore more standard C functions, variables, macros, and also Win32 API functions and macros.
No longer ignore "__int" C type: it should not be used in the doc.
📚 Documentation preview 📚: https://cpython-previews--107301.org.readthedocs.build/