GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-106948: Doc config ignores more standard C functions by vstinner · Pull Request #107301 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Jul 26, 2023 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

  • Issue: Docs: add standard external names to nitpick_ignore #106948

📚 Documentation preview 📚: https://cpython-previews--107301.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner
pythongh-106948: Doc config ignores more standard C functions
8cc4083 
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.
@vstinner
vstinner requested a review from a team as a code owner July 26, 2023 16:41
@bedevere-bot bedevere-bot mentioned this pull request Jul 26, 2023
Docs: add standard external names to nitpick_ignore #106948
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Jul 26, 2023
@vstinner

vstinner commented Jul 26, 2023

Copy link
Copy Markdown
Member Author

@erlend-aasland: In commit b447e19 (PR #107062), you decided to not add siginterrupt() to nitpick_ignore but use !siginterrupt syntax in the doc.

@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 sphinx-warnings.txt (after I removed all Doc/c-api/ from Doc/tools/.nitignore).

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 getopt() for example. I would prefer to use !function() syntax in the doc for these instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
Update Doc/tools/.nitignore
596198d
@vstinner vstinner mentioned this pull request Jul 26, 2023
gh-106948: Add Doc/nitpick_ignore.yml config file #107278
Closed
serhiy-storchaka
serhiy-storchaka reviewed Jul 26, 2023
View reviewed changes
Comment thread Doc/conf.py
('c:type', 'uintmax_t'),
('c:type', 'uintptr_t'),
('c:type', 'va_list'),
('c:type', 'wchar'),

@serhiy-storchaka serhiy-storchaka Jul 26, 2023

Copy link
Copy Markdown
Member

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. 😉

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with laugh emoji
All reactions
  • 😄 1 reaction

@vstinner vstinner Jul 26, 2023

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@erlend-aasland erlend-aasland Jul 26, 2023

Copy link
Copy Markdown
Contributor

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.

Suggested change
('c:type', 'wchar'),
('c:type', 'wchar_t'),

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
CAM-Gerlach reacted with eyes emoji
All reactions
  • 👀 1 reaction

@CAM-Gerlach CAM-Gerlach Jul 26, 2023

Copy link
Copy Markdown
Member

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:

Suggested change
('c:type', 'wchar'),

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka

serhiy-storchaka commented Jul 26, 2023

Copy link
Copy Markdown
Member

Smaller list is easier to maintain. A PR which adds less names is simpler to review (and yet somehow __int slipped through). There is less chance to use incorrect role with a name from the list if the list is smaller (COLS almost slipped through as an :envvar:). There is less chance to add a typo (like wchar). This is why I thought that we should add only the most common names (referred many times in more than one file) in the ignore list.

But if you and @erlend-aasland are fine with a longer list, I have no objections. Did you checked every one of these names?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Jul 26, 2023

Copy link
Copy Markdown
Contributor

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 !func markup for rarer functions and syscalls. A larger list will pollute conf.py, and as the docs change, it will inevitably contain more and more redundant entries.

I won't block this PR, but I won't approve it either ;)
CAM-Gerlach reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CAM-Gerlach CAM-Gerlach added docs Documentation in the Doc dir skip news labels Jul 26, 2023
CAM-Gerlach
CAM-Gerlach reviewed Jul 26, 2023
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/conf.py
('c:type', 'uintmax_t'),
('c:type', 'uintptr_t'),
('c:type', 'va_list'),
('c:type', 'wchar'),

@CAM-Gerlach CAM-Gerlach Jul 26, 2023

Copy link
Copy Markdown
Member

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:

Suggested change
('c:type', 'wchar'),

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Jul 26, 2023

Copy link
Copy Markdown
Member Author

Ok, I will try this approach. I started with PR #107329 which uses a few !func() syntax on standard C functions like memcpy() or pthread_atfork().
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner closed this Jul 26, 2023
@vstinner
vstinner deleted the doc_ignore_libc branch July 26, 2023 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@erlend-aasland erlend-aasland erlend-aasland left review comments
@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@vstinner @serhiy-storchaka @erlend-aasland @CAM-Gerlach @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.