Skip to content
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-85046: Document most errno constants #126420

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Nov 5, 2024

@bedevere-app bedevere-app bot added docs Documentation in the Doc dir skip news awaiting review labels Nov 5, 2024
@erlend-aasland erlend-aasland changed the title gh-85046: Complete missing items in errno documentation gh-85046: Document most errno constants Nov 6, 2024
@erlend-aasland
Copy link
Contributor

Before going further with this PR, please take examine closely the review remarks of #20665 and make sure all remarks are addressed here as well. Please also read the devguide regarding how to contribute documentation and make sure your PR aligns with the established guidelines.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Nov 6, 2024

I read the discussion again carefully. I will not complete the description of the WSA constant, because errno is only used to describe the content in linux/errno.h (which is already stated in the document, and there is no WSA in the document).

This module makes available standard errno system symbols. The value of each
symbol is the corresponding integer value. The names and descriptions are
borrowed from :file:linux/include/errno.h, which should be
all-inclusive.

@rruuaanng rruuaanng marked this pull request as draft November 6, 2024 13:29
@ZeroIntensity
Copy link
Member

In general, RUANG, try and get a PR to perfection locally before submitting it to us. Even as a draft, it's just extra noise on the issue tracker that can take our attention away from ready PRs.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Nov 6, 2024

I found some missing WSA constant documentation and am trying to locate their descriptions. Although I haven’t found them in the Windows development guidelines yet, I’m still searching. So, I need to convert it into a draft. (You can call me James; RUANG is just an alias. Emmm Okay. They are the same person.)

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does 3 things at the same time: add new error, document existing errors, change error messages. I would prefer to have one PR to each kind of change. I suggest to start with a single PR to add the new error (and close this one). (I'm not suggesting to open 3 PRs at the same time.)

@rruuaanng
Copy link
Contributor Author

To be honest, my initial PR was closed by Erlend. If separate commits are needed, perhaps you could help me reopen it, and I will handle these changes separately.

#126397

@erlend-aasland
Copy link
Contributor

As I said, @rruuaanng, the linked issue is about documentation, and documentation only. If more constants are to be added, IMO you should ideally open a feature request issue for those. I really do not like mixing features and bugfixes, neither in PRs nor in issues.

@rruuaanng
Copy link
Contributor Author

@erlend-aasland
Then we can start by reopening the closed PR as the first change (adding the new attribute). Once it's processed, we can move on to handling the other pending issues.
Do you think?

@erlend-aasland
Copy link
Contributor

@erlend-aasland Then we can start by reopening the closed PR as the first change (adding the new attribute). Once it's processed, we can move on to handling the other pending issues. Do you think?

If you want to add a new feature it is good practice to open an issue for the feature request first.

@erlend-aasland
Copy link
Contributor

And for the record: a new constant is a new feature. Any new API (including public constants) are features.

@rruuaanng
Copy link
Contributor Author

Okay. I will open an issue for this later. I will also change the title of #126397 to point to the new issue. Perhaps you can help me reopen it.

@erlend-aasland
Copy link
Contributor

Okay. I will open an issue for this later. I will also change the title of #126397 to point to the new issue. Perhaps you can help me reopen it.

Alternatively, instead of opening a new issue, you can focus this PR only on the task described in the linked issue:

Add the missing descriptions for the exiting errno constants.

In other words: only add documentation for the exiting errno contants. This implies that only .rst files are changed, and nothing else.

In addition to resolving the linked issue, this will also probably result in less reviewer friction and churn.

@rruuaanng
Copy link
Contributor Author

Okay. I will open an issue for this later. I will also change the title of #126397 to point to the new issue. Perhaps you can help me reopen it.

Alternatively, instead of opening a new issue, you can focus this PR only on the task described in the linked issue:

Add the missing descriptions for the exiting errno constants.

In other words: only add documentation for the exiting errno contants. This implies that only .rst files are changed, and nothing else.

In addition to resolving the linked issue, this will also probably result in less reviewer friction and churn.

I will roll back the PR so that it only fixes the missing description in the rst documentation. For the new feature, I think I will open a new issue, as you suggested. The lack of this attribute was actually a new problem discovered later in the current issue, and they did not open a new issue for it.

@rruuaanng
Copy link
Contributor Author

The current PR only deals with document missing items, and adding new features has been moved to the previously opened fix plan. Thank you for your attention to this event.

@rruuaanng rruuaanng marked this pull request as ready for review November 8, 2024 15:07
@rruuaanng
Copy link
Contributor Author

rruuaanng commented Nov 9, 2024

@vstinner Please review them! I'm pretty sure they're timed correctly :)

Edit
In addition, I use git blame and git log as tools, and confirm the versions corresponding to their submission years by calculating the release time of cpython version.

@vstinner
Copy link
Member

@vstinner Please review them!

As I wrote previously, I would prefer to have a PR just to add EHWPOISON constant first. I suggest to abandon this PR for now.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Nov 10, 2024

@vstinner Please review them!

As I wrote previously, I would prefer to have a PR just to add EHWPOISON constant first. I suggest to abandon this PR for now.

It has been opened and can convert this PR into a draft :)

Edit

In order to avoid spending time turning it on again after closing.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote:

As I wrote previously, I would prefer to have a PR just to add EHWPOISON constant first. I suggest to abandon this PR for now.

Oh, I didn't notice that you already modified your PR to only do one thing. In this case, it's acceptable. Ignore my previous comment.

Doc/library/errno.rst Outdated Show resolved Hide resolved
@rruuaanng
Copy link
Contributor Author

Neglecting is common :)

If there is no other problem with this PR, I hope you can approve it.

@ZeroIntensity ZeroIntensity added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Nov 10, 2024
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Doc/library/errno.rst Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor

I'll look this over tomorrow and land it.

@erlend-aasland erlend-aasland self-assigned this Nov 10, 2024
Doc/library/errno.rst Show resolved Hide resolved
Doc/library/errno.rst Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge docs Documentation in the Doc dir needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes skip news
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

5 participants