Skip to content

gh-99030: Added documentation links for types and exceptions #123857

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

Merged
merged 8 commits into from
Oct 21, 2024

Conversation

rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Sep 9, 2024

Copy link
Member

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution! Small suggestion!

Copy link
Contributor Author

@rruuaanng rruuaanng left a comment

Choose a reason for hiding this comment

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

That's so good, I think that's fine

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Sep 27, 2024

Hmmm, This PR has been open for almost a month now. I think we should make a decision soon, to merge or close it.

@skirpichev
Copy link
Member

@rruuaanng, be patient:)

@rruuaanng
Copy link
Contributor Author

@rruuaanng, be patient:)

Sorry skirpichev, but I saw on discuss that anything over two weeks might be considered for closing or marking as stale, which is concerning. Hmmm.

@skirpichev
Copy link
Member

skirpichev commented Sep 27, 2024

I saw on discuss that anything over two weeks might be considered for closing or marking as stale, which is concerning.

Could you point to that discussion?

First, we don't mark issues as stale at all. PRs marked as stale after 30 days of inactivity. But there is no rule that such PR should be closed. E.g. #109647 was merged after 3+ months, #25518 - after ~8. Just keep your pr in a good shape (fix merge conflicts).

Edit: @rruuaanng, I removed your request for review after the merge commit bb60ba1, as it applied cleanly. In general, please avoid "Update branch" button, unless you fix merge conflict or your PR is very old and you wish to trigger CI checks.

@skirpichev skirpichev removed the request for review from savannahostrowski September 27, 2024 05:14
@rruuaanng
Copy link
Contributor Author

I saw on discuss that anything over two weeks might be considered for closing or marking as stale, which is concerning.

Could you point to that discussion?

First, we don't mark issues as stale at all. PRs marked as stale after 30 days of inactivity. But there is no rule that such PR should be closed. E.g. #109647 was merged after 3+ months, #25518 - after ~8. Just keep your pr in a good shape (fix merge conflicts).

Edit: @rruuaanng, I removed your request for review after the merge commit bb60ba1, as it applied cleanly. In general, please avoid "Update branch" button, unless you fix merge conflict or your PR is very old and you wish to trigger CI checks.

Could you point to that discussion?

Seems like I can't bring up that discussion since it was something I stumbled upon accidentally, and maybe it's no longer there anyway.

Edit: @rruuaanng, I removed your request for review after the merge commit bb60ba1, as it applied cleanly. In general, please avoid "Update branch" button, unless you fix merge conflict or your PR is very old and you wish to trigger CI checks.

Oh well, thanks for handling the situation responsibly. Thanks skirpichev.

Comment on lines 11 to 12
:func:`open`. For more information, see :ref:`built-in-funcs`, :ref:`built-in-consts`,
:ref:`bltin-types` and :ref:`bltin-exceptions`.
Copy link
Contributor

Choose a reason for hiding this comment

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

A rendered lists of multiple links can be hard on the eye. I suggest instead to use two pieces of Sphinx functionality here:

  1. a list
  2. the seealso directive
Suggested change
:func:`open`. For more information, see :ref:`built-in-funcs`, :ref:`built-in-consts`,
:ref:`bltin-types` and :ref:`bltin-exceptions`.
:func:`open`.
.. seealso::
* :ref:`built-in-consts`
* :ref:`bltin-exceptions`
* :ref:`built-in-funcs`
* :ref:`bltin-types`

Consider also to add this directive at the very bottom of builtins.rst. IMO, it is too prominent at the very top.

cc. @python/editorial-board

Copy link
Contributor Author

@rruuaanng rruuaanng Oct 15, 2024

Choose a reason for hiding this comment

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

Actually, I don't think this needs to be changed because it needs to be a combination, not a list of separate introductions.

Edit:

This module provides direct access to all 'built-in' identifiers of Python; for
example, ``builtins.open`` is the full name for the built-in function
:func:`open`. For more information, see :ref:`built-in-funcs`, :ref:`built-in-consts`,
:ref:`bltin-types` and :ref:`bltin-exceptions`.
This module provides direct access to all 'built-in' identifiers of Python; for
example, ``builtins.open`` is the full name for the built-in function

:func:`open`.

.. seealso::

   * :ref:`built-in-consts`
   * :ref:`bltin-exceptions`
   * :ref:`built-in-funcs`
   * :ref:`bltin-types`

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that applying @erlend-aasland's suggestion makes good sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing my remark. I still think we should consider placing this seealso at the very bottom of the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Placing it at the very bottom of the document doesn't seem to be perfect, because it needs to be combined with the above text to form an example. For example, open(), and then the information related to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rruuaanng Thanks for applying the first suggestion. The second suggestion by @erlend-aasland recommends placing at the bottom of the page. Because this is a short doc file, I agree with Erlend that it makes sense to put at the end of the page. Please make the change to do as suggested.

Coaching tips for contributing to CPython

Your enthusiasm to contribute is causing extra work and reviews by the core team and triagers. To be a more effective contributor, I highly recommend:

  1. Slow down and observe. An effective contributor does not rush since many users depend on our code and its quality.
  2. Ask yourself whether a comment, issue, or PR is necessary or if they would cause unneeded code churn with only minor improvement.
  3. Study and follow the devguide, especially
  4. If a core team member suggests a change, they are doing so to improve your effectiveness as a contributor and to maintain the quality of the codebase.

Thanks in advance for slowing down, and I'm looking forward to improved contributions in the future. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I'm sorry, maybe it's my lack of experience. It seems that my vision is not as good as that of a core developer. I will continue to work hard and make useful contributions. Please forgive me for not being good at writing documents. Therefore, I will continue to work hard. Thank you for your guidance and erlend.
😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @rruuaanng. Over time, you will find that we are developing collaboratively, and the core developer's suggestions are to improve the code quality and maintainability. Learning to be an effective contributor takes time. Keep the end user in mind, and follow the coaching tips above and I am confident that you will have more success.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. After making @erlend-aasland's suggested changes, I would be happy to approve.

@rruuaanng
Copy link
Contributor Author

Thank for your reviewer!, Please reviewer again :)
@willingc

@rruuaanng rruuaanng requested a review from willingc October 18, 2024 03:09
@willingc
Copy link
Contributor

@rruuaanng The Lint is failing since there needs to be a newline at the end of the file.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks @rruuaanng for the PR and to @skirpichev @savannahostrowski and @erlend-aasland for taking the time to review. ☀️

@willingc willingc added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Oct 21, 2024
@willingc willingc merged commit 9256be7 into python:main Oct 21, 2024
28 checks passed
@miss-islington-app
Copy link

Thanks @rruuaanng for the PR, and @willingc for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 21, 2024
…ythonGH-123857)

* Added documentation links for types and exceptions

* Shortened description sentences

* Change content

* Change documentation

* Move seealso

* Add a spaces
(cherry picked from commit 9256be7)

Co-authored-by: RUANG (Roy James) <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 21, 2024
…ythonGH-123857)

* Added documentation links for types and exceptions

* Shortened description sentences

* Change content

* Change documentation

* Move seealso

* Add a spaces
(cherry picked from commit 9256be7)

Co-authored-by: RUANG (Roy James) <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 21, 2024

GH-125764 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 21, 2024
@bedevere-app
Copy link

bedevere-app bot commented Oct 21, 2024

GH-125765 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 21, 2024
willingc pushed a commit that referenced this pull request Oct 21, 2024
…H-123857) (#125764)

gh-99030: Added documentation links for types and exceptions (GH-123857)

* Added documentation links for types and exceptions

* Shortened description sentences

* Change content

* Change documentation

* Move seealso

* Add a spaces
(cherry picked from commit 9256be7)

Co-authored-by: RUANG (Roy James) <[email protected]>
willingc pushed a commit that referenced this pull request Oct 21, 2024
…H-123857) (GH-125765)

gh-99030: Added documentation links for types and exceptions (GH-123857)

* Added documentation links for types and exceptions

* Shortened description sentences

* Change content

* Change documentation

* Move seealso

* Add a spaces
(cherry picked from commit 9256be7)

Co-authored-by: RUANG (Roy James) <[email protected]>
@rruuaanng rruuaanng deleted the dev3 branch October 21, 2024 02:41
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
…ythonGH-123857)

* Added documentation links for types and exceptions

* Shortened description sentences

* Change content

* Change documentation

* Move seealso

* Add a spaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants