Skip to content

bpo-19184: Update the documentation of dis module. #13652

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 5 commits into from
Jun 2, 2019

Conversation

mangrisano
Copy link
Contributor

@mangrisano mangrisano commented May 29, 2019

  • Explain the behavior of the number of arguments of RAISE_VARGARGS
    opcode.

https://bugs.python.org/issue19184

* Explain the behavior of the number of arguments of RAISE_VARGARGS
  opcode.
@asvetlov asvetlov changed the title bpo-19184: Update the documentation of fis module. bpo-19184: Update the documentation of dis module. May 29, 2019
@csabella csabella requested review from berkerpeksag and ncoghlan May 29, 2019 12:21
@csabella
Copy link
Contributor

Thanks for the contribution, @mangrisano! The CI failures are on the lines you added, but I have to research what the error means as I haven't encountered it before.

$ make check suspicious html SPHINXOPTS="-q -W -j4"
python3 tools/rstlint.py -i tools -i ./venv -i README.rst
[2] library/dis.rst:1118: default role used
[2] library/dis.rst:1120: default role used
[2] library/dis.rst:1121: default role used
[2] library/dis.rst:1122: default role used
[2] library/dis.rst:1123: default role used
5 problems with severity 2 found.
Makefile:183: recipe for target 'check' failed
make: *** [check] Error 1

@mangrisano
Copy link
Contributor Author

mangrisano commented May 29, 2019

Thanks for the contribution, @mangrisano! The CI failures are on the lines you added, but I have to research what the error means as I haven't encountered it before.

$ make check suspicious html SPHINXOPTS="-q -W -j4"
python3 tools/rstlint.py -i tools -i ./venv -i README.rst
[2] library/dis.rst:1118: default role used
[2] library/dis.rst:1120: default role used
[2] library/dis.rst:1121: default role used
[2] library/dis.rst:1122: default role used
[2] library/dis.rst:1123: default role used
5 problems with severity 2 found.
Makefile:183: recipe for target 'check' failed
make: *** [check] Error 1

The problem concerned the "`" when it wanted "``". First time for me as well. (Thanks Ezio for taking a look about it)

@csabella
Copy link
Contributor

Thanks for fixing that so quickly; saved me the time. 🙂

Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

Mostly cosmetic issues, otherwise it looks pretty good to me. Thanks!

@@ -0,0 +1,3 @@
Update the documentation of :mod:`dis`:
Copy link
Member

Choose a reason for hiding this comment

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

This is a minor documentation change. We don't need to document it in a NEWS file. We only require NEWS entries for major documentation changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok. I didn't know that. Thank you for the review.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ezio-melotti
Copy link
Member

Thanks for the contribution, @mangrisano! The CI failures are on the lines you added, but I have to research what the error means as I haven't encountered it before.

$ make check suspicious html SPHINXOPTS="-q -W -j4"
python3 tools/rstlint.py -i tools -i ./venv -i README.rst
[2] library/dis.rst:1118: default role used
...

To expand a bit on this: rest uses roles in the form of :name:`value` for inline markup (e.g. :func:`any`). The default role in the message refers to the "unnamed" default role, i.e. `value`, and we don't use that in the Python docs. (Not to be confused with ``value``, used for inline snippets of code).

The devguide talks about roles at https://devguide.python.org/documenting/#id4, but doesn't mention anything about the default role -- perhaps a PR to add an item to that list should be created.

@csabella
Copy link
Contributor

Thanks @ezio-melotti! I wonder if it would help to add a cheat sheet (or FAQ) to the devguide too? For example, if you see x on the build then do y. Your explanation would fit in well with something like that and save a lot of time in troubleshooting.

@miss-islington
Copy link
Contributor

Thanks @mangrisano for the PR, and @ezio-melotti for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

I'm having trouble backporting to 3.7. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.7 label.

@miss-islington
Copy link
Contributor

Thanks @mangrisano for the PR, and @ezio-melotti for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 2, 2019
* bpo-19184: Update the documentation of dis module

* Explain the behavior of the number of arguments of RAISE_VARGARGS
  opcode.

* bpo-19184: Update blurb.

* bpo-19184: Fix typo in the dis Documentation.

* bpo-19184: Address review comments and improve the doc

* bpo-19184: Remove news file.
(cherry picked from commit e1179a5)

Co-authored-by: Michele Angrisano <[email protected]>
@bedevere-bot
Copy link

GH-13755 is a backport of this pull request to the 3.7 branch.

@mangrisano mangrisano deleted the fix-issue-19184 branch June 2, 2019 21:37
ezio-melotti pushed a commit that referenced this pull request Jun 2, 2019
* bpo-19184: Update the documentation of dis module

* Explain the behavior of the number of arguments of RAISE_VARGARGS
  opcode.

* bpo-19184: Update blurb.

* bpo-19184: Fix typo in the dis Documentation.

* bpo-19184: Address review comments and improve the doc

* bpo-19184: Remove news file.
(cherry picked from commit e1179a5)

Co-authored-by: Michele Angrisano <[email protected]>
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
* bpo-19184: Update the documentation of dis module

* Explain the behavior of the number of arguments of RAISE_VARGARGS
  opcode.

* bpo-19184: Update blurb.

* bpo-19184: Fix typo in the dis Documentation.

* bpo-19184: Address review comments and improve the doc

* bpo-19184: Remove news file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants