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-92417: Update docs and examples of doctest.IGNORE_EXCEPTION_DETAIL for Py>=3 #92502

Merged
merged 3 commits into from
May 19, 2022

Conversation

CAM-Gerlach
Copy link
Member

@CAM-Gerlach CAM-Gerlach commented May 8, 2022

Part of issue #92417 and followup to @AlexWaygood 's PR #92420 .

Following #92420 , there's still a chunk of text in the doctest library docs referring to compatibility with Python <3.2, and workarounds to support Python <2.4, in the description of doctest.EXCEPTION_DETAIL which it seems should be elided as well.

It substantially complicates the description of doctest.IGNORE_EXCEPTION_DETAIL and doesn't seem to be of any use for modern Python, beyond what is already noted in the versionchanged notice. Furthermore, it made the section pretty difficult to understand and parse for me even as a native English speaker, technical writer/copyeditor and reasonably experianced Python developer; I had to re-read it several times to fully grasp the intended meaning.

Therefore, I simplified the section to remove the outdated material and focus on the functionality relevant to modern Python, while retaining the existing versionchanged note that already captured the key version differences clearly.

Sidenote, I'm a triager now, so I'm not sure why I still can't set the PR tags and such when creating the PR, but can perfectly fine after it is created (causing the skip-news check to initially fail and resulting in additional noise on the PR).

@AlexWaygood
Copy link
Member

I'd like to review this — @JelleZijlstra, can you give me a day or two before merging? :)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I believe that the example on lines 576-582 is meant to illustrate how this option creates cross-compatibility between Python 2 and 3. In Python 3, the fully qualified name of the exception is, I believe, always shown:

>>> class CustomError(Exception): ...
...
>>> raise CustomError('message')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
__main__.CustomError: message

So, maybe that should also go.

Doc/library/doctest.rst Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member

AlexWaygood commented May 8, 2022

In general, I'm worried that this part of the doctest API seems like it might be there only to provide cross-compatibility between Python 2 and Python 3. If that's the case, rather than just deleting all documentation about it, we should maybe just deprecate the feature?

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented May 8, 2022

To respond to your high-level feedback, it took several re-readings of the original to ensure I was clear, but unless I'm grossly misunderstanding it, this overall feature seems to have substantially broader applicability than just differences between older Python versions (and newer/future Python versions, for that matter).

Just for the built-in exception classes, error messages may vary on newer Python versions as well (as there has been a comprehensive effort in the past couple to rewrite them to be clearer and more understandable), potentially even across bugfix releases. Beyond that, non-builtin exceptions may change their error messages at any time, which may or may not be under control of the doctest author. And there could also be situations where part of the error message may not be entirely deterministic, or may intentionally not match the exact output for illustrative purposes.

Therefore, this feature would seem to have significant utility beyond one particular example of older Python version differences (minus the examples and guidance I removed that were specific to older Python versions). To avoid this, we could introduce a new option, IGNORE_EXCEPTION_MESSAGE, that just ignores the message but not the module (as IGNORE_EXCEPTION_DETAIL originally did), but that call would be above my pay grade.

The outstanding use case for ignoring the module name of the exception appears to be smaller, but still arguably present; it should mean doctests would not fail if the location of the exception class was refactored to a different or renamed module across versions of Python, a third-party library or the user's own code, so long as its class was the same. There may be an argument to be made that this lack of strictness may be less desirable than just differences in the displayed text, but that would be a backwards-incompatible change that could break existing passing doctests, so there would need to be a compelling benefit to remove it which I'm not sure is worth it.

I see @tim-one is marked as the (inactive) doctest maintainer in the Experts Index, and from git blame it looks like @ncoghlan committed the original addition based on an initial patch by @regebro . Pinging them to see if they have any insight on this.

@rhettinger rhettinger requested a review from tim-one May 9, 2022 08:55
@rhettinger rhettinger removed the needs backport to 3.9 only security fixes label May 9, 2022
@CAM-Gerlach
Copy link
Member Author

@rhettinger Could you explain why the needs backport to 3.9 label was removed? Both 3.10 and 3.9 are still in bugfix mode, the change only affects compatibility with Python releases (2.3 and 2.7) that were EoL back when 3.9 was first released, and besides beyond just removing very-outdated material, it fixes a substantive defect given said explanations make the documentation difficult to understand and follow, as evidenced by both @AlexWaygood and my own initial confusion over what specifically this option did. Thanks.

@regebro
Copy link

regebro commented May 13, 2022

IIRC the IGNORE_EXCEPTION_* things were indeed added exactly for 2/3 compatibility, but I agree with you, it's also useful in cases where a module gets refactored, or where maybe you support a future.module import etc, so I think it shouldn't be removed completely.

The changes now look reasonable to me.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 13, 2022

@CAM-Gerlach, I'd prefer to wait for Tim's feedback, since he's been requested for review :)

I feel like if this feature is still relevant for Python 3, we should probably replace the Python 2 examples with Python 3 examples. But most of the docs around this feature seem to currently be focused around py2/3 compatibility, and I don't have enough experience with the feature to really suggest any alternatives.

@CAM-Gerlach
Copy link
Member Author

I feel like if this feature is still relevant for Python 3, we should probably replace the Python 2 examples with Python 3 examples. But most of the docs around this feature seem to currently be focused around py2/3 compatibility, and I don't have enough experience with the feature to really suggest any alternatives.

The example for message is already fully version-agnostic; I've tweaked the example for module names to be more appropriate and well as implemented the wording revision to the Ellipses reference suggested in response to your comment above to match. The remaining examples that were removed were just specific workarounds for earlier Py<3 versions that are no longer needed/relevant.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd still prefer to wait a little longer to see if Tim has any feedback :)


>>> raise CustomError('message')
Traceback (most recent call last):
CustomError: message
my_module.CustomError: message
Copy link
Member

Choose a reason for hiding this comment

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

Please keep an example without module name.

The module name is not output if it is "builtins" or "__main__". Some extension types can have __module__ incorrectly set to "builtins", because it is the default, it is not rare case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks; I included examples both with, without and with a different module name, using the plausible real-world examples you list above.

It will also ignore the module name used in Python 3 doctest reports. Hence
both of these variations will work with the flag specified, regardless of
whether the test is run under Python 2.7 or Python 3.2 (or later versions)::
It will also ignore the module name used in doctest reports;
Copy link
Member

Choose a reason for hiding this comment

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

Technically, it ignores not only the module name, but the full qualified name except the name of the exception type (the last component). I would perhaps merge it with the previous paragraph: "... the exception detail (such as the full qualified name or arguments) ..."

It may be worth to explain why this directive is needed: the full qualified name and message can be different between different versions of Python or other libraries and between the Python and the C implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks; I rewrote the section accordingly to clarify the points you raised and explain why it is needed. I also clarified some potentially confusing/hard to understand phrasing and fixed the code block syntax highlighting, among other minor improvements.

@CAM-Gerlach CAM-Gerlach changed the title gh-92417: Remove workarounds for Py <3 compat in doctest docs gh-92417: Update description and examples of doctest.IGNORE_EXCEPTION_DETAIL for Py>=3 May 14, 2022
@CAM-Gerlach CAM-Gerlach changed the title gh-92417: Update description and examples of doctest.IGNORE_EXCEPTION_DETAIL for Py>=3 gh-92417: Update docs and examples of doctest.IGNORE_EXCEPTION_DETAIL for Py>=3 May 14, 2022
@AlexWaygood AlexWaygood removed their request for review May 14, 2022 23:15
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

This looks good to me as it explains the feature clearly without talking about long-dead Python 2.3 compatibility. I'll leave merging this to others though.

@ambv ambv merged commit 97b9c10 into python:main May 19, 2022
@miss-islington
Copy link
Contributor

Thanks @CAM-Gerlach for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10, 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-92962 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label May 19, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2022
…DETAIL for Py>=3 (pythonGH-92502)

(cherry picked from commit 97b9c10)

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2022
…DETAIL for Py>=3 (pythonGH-92502)

(cherry picked from commit 97b9c10)

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@bedevere-bot
Copy link

GH-92963 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label May 19, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2022
…DETAIL for Py>=3 (pythonGH-92502)

(cherry picked from commit 97b9c10)

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label May 19, 2022
@bedevere-bot
Copy link

GH-92964 is a backport of this pull request to the 3.9 branch.

ambv pushed a commit that referenced this pull request May 19, 2022
ambv pushed a commit that referenced this pull request May 19, 2022
ambv pushed a commit that referenced this pull request May 19, 2022
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.

9 participants