Skip to content

bpo-19225: Lack of c api exceptions doc #881

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 2, 2017
Merged

bpo-19225: Lack of c api exceptions doc #881

merged 7 commits into from
Apr 2, 2017

Conversation

cocoatomo
Copy link
Contributor

see bpo-19225.

I create this patch based on the patch created by Tamás Bence Gedai (beng94).

@mention-bot
Copy link

@cocoatomo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @DanielStutzbach and @benjaminp to be potential reviewers.

@cocoatomo cocoatomo mentioned this pull request Mar 29, 2017
Closed
methane
methane reviewed Mar 29, 2017
View reviewed changes
Doc/c-api/exceptions.rst Outdated
| :c:data:`PyExc_BaseException` | :exc:`BaseException` | \(1) |
+-----------------------------------------+---------------------------------+----------+
| :c:data:`PyExc_Exception` | :exc:`Exception` | \(1) |
+-----------------------------------------+---------------------------------+----------+
Copy link
Member

Choose a reason for hiding this comment

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

I feel BaseException and Exception are special enough to ignore alphabetical order.

Copy link
Member

Choose a reason for hiding this comment

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

And the same is true for Warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

serhiy-storchaka
serhiy-storchaka reviewed Mar 29, 2017
View reviewed changes
Doc/c-api/exceptions.rst Outdated
| :c:data:`PyExc_BaseException` | :exc:`BaseException` | \(1) |
+-----------------------------------------+---------------------------------+----------+
| :c:data:`PyExc_Exception` | :exc:`Exception` | \(1) |
+-----------------------------------------+---------------------------------+----------+
| :c:data:`PyExc_ArithmeticError` | :exc:`ArithmeticError` | \(1) |
Copy link
Member

Choose a reason for hiding this comment

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

Since that are definitions of PyExc_* names, wouldn't be better to use the c:var directive?

.. c:var:: PyExc_ArithmeticError

Perhaps the index directive below will be not needed after that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this table is the most suitable place for definition of PyExc_* names.
I would fix these markup and delete index directives.

Doc/c-api/exceptions.rst Outdated
| :c:data:`PyExc_BaseException` | :exc:`BaseException` | \(1) |
+-----------------------------------------+---------------------------------+----------+
| :c:data:`PyExc_Exception` | :exc:`Exception` | \(1) |
+-----------------------------------------+---------------------------------+----------+
Copy link
Member

Choose a reason for hiding this comment

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

And the same is true for Warning.

Doc/c-api/exceptions.rst Outdated
Notes:

(1)
This is a base class for other standard exceptions.
Copy link
Member

Choose a reason for hiding this comment

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

... for other standard warning categories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@cocoatomo
Copy link
Contributor Author

Thanks for your good advisement. I updated the pull request.

@MSeifert04
Copy link
Contributor

MSeifert04 commented Mar 29, 2017 •
edited
Loading

The tables will be really huge with the .. c:var: PyExc[...] directives. I tried it a few days ago but haven't suggested it because it almost doubled the vertical extend (compared to the current formatting) of the table:

unbenannt

@serhiy-storchaka
Copy link
Member

Hmm, then it would be better to restore the variant with :c:data: and separate .. index::.

@cocoatomo
Copy link
Contributor Author

I changed .. c:var:: back to :c:data:and restore .. index:: in the more appropriate position.

@brettcannon brettcannon added the docs Documentation in the Doc dir label Mar 30, 2017
@cocoatomo
Copy link
Contributor Author

@methane @serhiy-storchaka @MSeifert04
Is any additional work needed for this pull request?

@serhiy-storchaka
Copy link
Member

Move PyExc_BaseException, PyExc_Exception and PyExc_Warning at the top of corresponding tables.

@cocoatomo
Copy link
Contributor Author

Thanks for your comment.
I made a mistake at the commit a0e3bb7.

@serhiy-storchaka serhiy-storchaka merged commit e8c7631 into python:master Apr 2, 2017
This was referenced Apr 2, 2017
Merged
Merged
Merged
Mariatta pushed a commit to Mariatta/cpython that referenced this pull request Apr 3, 2017
@cocoatomo @Mariatta
[3.6]: bpo-19225: Add a table of warning names and missed exception n…
e2bbcb8 
…ames in C API doc (pythonGH-881)

Move the `.. index` directive to more appropriate place.
(cherry picked from commit e8c7631)
@Mariatta Mariatta mentioned this pull request Apr 3, 2017
Closed
Mariatta pushed a commit to Mariatta/cpython that referenced this pull request Apr 3, 2017
@cocoatomo @Mariatta
[3.5]: bpo-19225: Add a table of warning names and missed exception n…
22fdd8b 
…ames in C API doc (pythonGH-881)

Move the `.. index` directive to more appropriate place..
(cherry picked from commit e8c7631)
@Mariatta Mariatta mentioned this pull request Apr 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@methane methane methane left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
docs Documentation in the Doc dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cocoatomo @mention-bot @MSeifert04 @serhiy-storchaka @methane @brettcannon @zhangyangyu @Mariatta @the-knights-who-say-ni