GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
|
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
3.6 is in security-fix-only mode, so this definitely will not be backported that far. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Oh oops, for some reason I had forgotten that 3.6 was a security-only branch. Thanks for clarifying. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Break up long line to improve readability. Co-Authored-By: Zachary Ware <[email protected]>
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.
|
Any update on this? Should I be looking at the 3.7 and 3.8 branches for backporting? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I believe at this point we're just waiting for a follow-up review from @zware.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| sys.modules.update(self.modules_before) | ||
|
|
||
|
|
||
| @contextlib.contextmanager |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
|
@aeros Thanks for the update and for explaining how the backport process works. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@zware is there any update on this? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| >>> sys.argv = save_argv | ||
| """ | ||
|
|
||
| class TestLoader(importlib.abc.MetaPathFinder, importlib.abc.ResourceLoader): |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
Change the name of the class used to test the case of a package with a loader.get_data method from TestLoader to TestImporter.
|
Any update on merging this? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I refreshed the review request for @zware to see if he has time to review again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…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]>
|
GH-19175 is a backport of this pull request to the 3.8 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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]>
|
GH-19176 is a backport of this pull request to the 3.7 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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]>
…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]>
|
@zware @brettcannon Thanks so much for getting this merged! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.