GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method. by pdonis · Pull Request #17385 · python/cpython

Conversation

@pdonis

@pdonis pdonis commented Nov 26, 2019 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method.

This pull request fixes the newline conversion bug originally reported in issue 1812. When that issue was originally submitted, the open builtin did not default to universal newline mode; now it does, which makes the issue fix simpler, since the only code path that needs to be changed is the one in doctest._load_testfile where the file is loaded from a package whose loader has a get_data method. Commit b6ad8ee contains the change that implements the fix.

This pull request also adds a test that verifies that doctest.testfile does the newline conversion correctly when loading from a package whose loader has a get_data method. This test fails (a ValueError is raised for inconsistent whitespace) unless the change in commit b6ad8ee is applied. Commit dd02845 contains the changes that add the test; the test is added to the test_lineendings docstring along with supporting comments.

Also, the test_lineendings docstring is missing a test to check for correct handling of the old-style Mac newline \r. This pull request adds that case to the original test_lineendings docstring. Commit da35562 contains the changes that add this test and edit the surrounding comments accordingly.

Finally, this pull request contains commit 22094c8 that adds my name to the MISC/Acks file.

https://bugs.python.org/issue1812

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
pdonis added 7 commits November 23, 2019 23:35
@pdonis
bpo-1812: Add old-style Mac newline test to test_doctest.test_lineend…
da35562
…ings.

The test_lineendings test in test_doctest did not include a test with
old-style Mac line endings (\r). This commit adds one, and edits the
introductory discussion in test_lineendings accordingly.
@pdonis
bpo-1812: Add test to test_doctest.test_lineendings that loads testfi…
dd02845
…le from package with loader.get_data.

The previous test_doctest.test_lineendings did not cover the case where a testfile for doctest.testfile
is loaded from a package whose loader has a get_data method. This commit adds a test for that case.
@pdonis
Merge branch 'master' into fix-issue-1812
59370c5
@pdonis
bpo-1812: Fix newline conversion when loading doctest testfile from p…
b6ad8ee
…ackage with loader.get_data.

This commit corrects the newline conversion that is done in doctest._load_testfile when the file
is being loaded from a package whose loader has a get_data method.
@pdonis
Add Peter Donis to Misc/ACKS.
22094c8
@pdonis
Merge branch 'master' into fix-issue-1812
80977d7
@pdonis
Merge branch 'master' into fix-issue-1812
74471ec
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 26, 2019
@bedevere-bot bedevere-bot added the awaiting review label Nov 26, 2019
@pdonis pdonis changed the title Fix issue 1812 bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method. Nov 26, 2019
@pdonis
Add MISC/NEWS.d blurb.
83c03d8
zware
zware reviewed Feb 12, 2020
View reviewed changes

@zware zware left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Overall, this is an incredibly clean patch, and you have the patience of a saint to have shepherded this along for 12 years! I have a style nit and a cleanup suggestion, but this is probably acceptable as is.

Thanks for the patch, and hopefully we'll finally get this landed!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_doctest.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_doctest.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@gpshead gpshead added the type-bug An unexpected behavior, bug, or error label Feb 12, 2020
@aeros

aeros commented Feb 12, 2020

Copy link
Copy Markdown
Contributor

This seems like it should be backported to 3.6+, no? It's fixing an internal function and making some changes to the tests, so I don't think there's any backwards compatibility concerns to worry about. I'll add the backport labels for now, but feel free to remove them if needed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aeros aeros added needs backport to 3.6 labels Feb 12, 2020
@zware

zware commented Feb 12, 2020

Copy link
Copy Markdown
Member

3.6 is in security-fix-only mode, so this definitely will not be backported that far.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zware zware removed the needs backport to 3.6 label Feb 12, 2020
@aeros

aeros commented Feb 12, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Oh oops, for some reason I had forgotten that 3.6 was a security-only branch. Thanks for clarifying.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pdonis and others added 3 commits February 12, 2020 19:51
@pdonis @zware
Update Lib/test/test_doctest.py
efabd42
Break up long line to improve readability.

Co-Authored-By: Zachary Ware <[email protected]>
@pdonis
Fix typo in commit efabd42.
1d56c05
@pdonis
bpo-1812: Wrap package hook test in context manager.
e1f0a2f
This commit wraps the doctest.testfile test with a package loader that
has a get_data method in a context manager to ensure proper removal
of the extra package hook.
@pdonis

pdonis commented Feb 21, 2020

Copy link
Copy Markdown
Contributor Author

Any update on this? Should I be looking at the 3.7 and 3.8 branches for backporting?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aeros

aeros commented Feb 21, 2020

Copy link
Copy Markdown
Contributor

Any update on this?

I believe at this point we're just waiting for a follow-up review from @zware.

Should I be looking at the 3.7 and 3.8 branches for backporting?

The labels I added will trigger @miss-islington to attempt to open backport PRs to the 3.8 and 3.7 branches after this one is merged. It typically works without conflicts, but if there are any, you may have to resolve them manually with git cherry-pick (or the tool on PyPI, cherry-picker).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ammaraskar
ammaraskar reviewed Feb 21, 2020
View reviewed changes
Comment thread Lib/doctest.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

ammaraskar
ammaraskar reviewed Feb 21, 2020
View reviewed changes
Comment thread Lib/test/test_doctest.py
sys.modules.update(self.modules_before)


@contextlib.contextmanager

@ammaraskar ammaraskar Feb 21, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think some of this and the code above can be folded to use stuff from https://github.com/python/cpython/blob/master/Lib/test/test_importlib/fixtures.py like:

  • cpython/Lib/test/test_importlib/fixtures.py

    Lines 71 to 83 in fd5116c

    class OnSysPath(Fixtures):
    @staticmethod
    @contextlib.contextmanager
    def add_sys_path(dir):
    sys.path[:0] = [str(dir)]
    try:
    yield
    finally:
    sys.path.remove(str(dir))
    def setUp(self):
    super(OnSysPath, self).setUp()
    self.fixtures.enter_context(self.add_sys_path(self.site_dir))

  • cpython/Lib/test/test_importlib/fixtures.py

    Lines 50 to 56 in fd5116c

    @contextlib.contextmanager
    def install_finder(finder):
    sys.meta_path.append(finder)
    try:
    yield
    finally:
    sys.meta_path.remove(finder)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@pdonis pdonis Feb 22, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The OnSysPath fixture appears to be designed for a unittest test case, not doctest. A standalone version of the add_sys_path context manager might work if combined with install_finder; however, I notice that neither of those clears the path importer cache, and neither one removes the imported test module from sys.modules when the test is done. I don't know enough about the internals of the test framework to know if leaving out those last two items is OK; I put them in to make sure that everything that could possibly be mutated by this particular test was restored afterwards.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@pdonis
Merge branch 'master' into fix-issue-1812
4664f1e
@pdonis

pdonis commented Feb 22, 2020

Copy link
Copy Markdown
Contributor Author

@aeros Thanks for the update and for explaining how the backport process works.

aeros reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pdonis

pdonis commented Mar 1, 2020

Copy link
Copy Markdown
Contributor Author

@zware is there any update on this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brettcannon
brettcannon reviewed Mar 13, 2020
View reviewed changes
Comment thread Lib/test/test_doctest.py Outdated
>>> sys.argv = save_argv
"""

class TestLoader(importlib.abc.MetaPathFinder, importlib.abc.ResourceLoader):

@brettcannon brettcannon Mar 13, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is misnamed as this is implementing a finder and a loader, so call it an importer.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@pdonis pdonis Mar 16, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'll push an update changing the name to TestImporter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@pdonis pdonis Mar 16, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Commit bb836d7 does the name change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
pdonis added 2 commits March 16, 2020 14:17
@pdonis
Merge branch 'master' into fix-issue-1812
860f835
@pdonis
bpo-1812: Change TestLoader to TestImporter.
bb836d7
Change the name of the class used to test the case of a package with
a loader.get_data method from TestLoader to TestImporter.
@pdonis

pdonis commented Mar 24, 2020

Copy link
Copy Markdown
Contributor Author

Any update on merging this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brettcannon
brettcannon requested a review from zware March 24, 2020 19:06
@brettcannon

brettcannon commented Mar 24, 2020

Copy link
Copy Markdown
Member

I refreshed the review request for @zware to see if he has time to review again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zware
zware approved these changes Mar 26, 2020
View reviewed changes

@zware zware left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I wouldn't mind seeing the cleanup code cleaned up a bit in a separate PR, but as nobody has pointed out any actual problems with it as is, let's go ahead.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/doctest.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_doctest.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Mar 26, 2020
@zware
zware merged commit e0b8101 into python:master Mar 26, 2020
@miss-islington

miss-islington commented Mar 26, 2020

Copy link
Copy Markdown
Contributor

Thanks @pdonis for the PR, and @zware for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting merge label Mar 26, 2020
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 26, 2020
@pdonis @miss-islington
bpo-1812: Fix newline conversion when doctest.testfile loads from a p…
c98f1f5
…ackage whose loader has a get_data method (pythonGH-17385)

This pull request fixes the newline conversion bug originally reported in bpo-1812. When that issue was originally submitted, the open builtin did not default to universal newline mode; now it does, which makes the issue fix simpler, since the only code path that needs to be changed is the one in doctest._load_testfile where the file is loaded from a package whose loader has a get_data method.
(cherry picked from commit e0b8101)

Co-authored-by: Peter Donis <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.8 label Mar 26, 2020
@bedevere-bot

bedevere-bot commented Mar 26, 2020

Copy link
Copy Markdown

GH-19175 is a backport of this pull request to the 3.8 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 26, 2020
@pdonis @miss-islington
bpo-1812: Fix newline conversion when doctest.testfile loads from a p…
a24ef3c
…ackage whose loader has a get_data method (pythonGH-17385)

This pull request fixes the newline conversion bug originally reported in bpo-1812. When that issue was originally submitted, the open builtin did not default to universal newline mode; now it does, which makes the issue fix simpler, since the only code path that needs to be changed is the one in doctest._load_testfile where the file is loaded from a package whose loader has a get_data method.
(cherry picked from commit e0b8101)

Co-authored-by: Peter Donis <[email protected]>
@bedevere-bot

bedevere-bot commented Mar 26, 2020

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.7 label Mar 26, 2020
miss-islington added a commit that referenced this pull request Mar 26, 2020
@miss-islington @pdonis
bpo-1812: Fix newline conversion when doctest.testfile loads from a p…
b05fbe9
…ackage whose loader has a get_data method (GH-17385)

This pull request fixes the newline conversion bug originally reported in bpo-1812. When that issue was originally submitted, the open builtin did not default to universal newline mode; now it does, which makes the issue fix simpler, since the only code path that needs to be changed is the one in doctest._load_testfile where the file is loaded from a package whose loader has a get_data method.
(cherry picked from commit e0b8101)

Co-authored-by: Peter Donis <[email protected]>
miss-islington added a commit that referenced this pull request Mar 26, 2020
@miss-islington @pdonis
bpo-1812: Fix newline conversion when doctest.testfile loads from a p…
9387678
…ackage whose loader has a get_data method (GH-17385)

This pull request fixes the newline conversion bug originally reported in bpo-1812. When that issue was originally submitted, the open builtin did not default to universal newline mode; now it does, which makes the issue fix simpler, since the only code path that needs to be changed is the one in doctest._load_testfile where the file is loaded from a package whose loader has a get_data method.
(cherry picked from commit e0b8101)

Co-authored-by: Peter Donis <[email protected]>
@pdonis

pdonis commented Mar 26, 2020

Copy link
Copy Markdown
Contributor Author

@zware @brettcannon Thanks so much for getting this merged!

aeros reacted with hooray emoji brettcannon reacted with heart emoji
All reactions
  • 🎉 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pdonis
pdonis deleted the fix-issue-1812 branch March 28, 2020 03:11
@merwok merwok mentioned this pull request Jan 28, 2021
bpo-43049: Use io.IncrementalNewlineDecoder for doctest newline conversion #24359
Merged
zware pushed a commit that referenced this pull request Mar 2, 2021
@pdonis
bpo-43049: Use io.IncrementalNewlineDecoder for doctest newline conve…
b36349a
…rsion (GH-24359)

Followup to bpo-1812 and GH-17385.
@sthagen sthagen mentioned this pull request Mar 2, 2021
bpo-43049: Use io.IncrementalNewlineDecoder for doctest newline conve… sthagen/python-cpython#412
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon left review comments
@ammaraskar ammaraskar ammaraskar left review comments
@zware zware zware approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@pdonis @aeros @zware @bedevere-bot @brettcannon @miss-islington @ammaraskar @gpshead @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.