{Refactor} Remove unnecessary cli_ctx reference#213
Merged
jiasli merged 1 commit intomicrosoft:masterfrom Dec 16, 2020
Merged
Conversation
Steg17
approved these changes
Sep 17, 2020
evelyn-ys
approved these changes
Dec 15, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Remove unnecessary
cli_ctxreference, which makespytest --capture=nofail:Root cause
When
pytestcapturesstdout,pytestusesio.FileIOto patchsys.stdoutCLI,CLI.out_filepoints toio.FileIOobjectknack/knack/cli.py
Line 33 in 0d0308b
tests.util.redirect_iousesio.StringIOto patchsys.stdout, but this doesn't affect the existingCLI.out_fileSince
CLI.out_filedoesn't equalsys.__stdout__(the originalstdout), it won't be replaced with acolorama.ansitowin32.StreamWrapper:knack/knack/cli.py
Lines 195 to 197 in 0d0308b
As
PreviewItem(same forExperimentalItemandDeprecated) keeps a reference tocli_ctx, during invocationknack/knack/commands.py
Line 353 in fe3bf5d
PreviewItemis deepcopied ->cli_ctxis deepcopied ->CLI.out_fileis deepcopiedBecause
io.FileIOcan't be pickled,copy.deepcopyfails and reachesknack/knack/util.py
Lines 78 to 82 in 5f0bcc2
This makes
cli_ctxnot being deepcopied.However, when
pytestis called withpytest --capture=no, it doesn't patchsys.stdout,CLI,CLI.out_fileequalssys.__stdout__tests.util.redirect_iousesio.StringIOto patchsys.stdout, but this doesn't affect the existingCLI.out_fileSince
CLI.out_fileequalssys.__stdout__(the originalstdout), it will be replaced with acolorama.ansitowin32.StreamWrapperand be deepcopied.Because of a circular reference in
AnsiToWin32https://github.com/tartley/colorama/blob/3d8d48a95de10be25b161c914f274ec6c41d3129/colorama/ansitowin32.py#L28-L29
and
io.StringIOcan be pickled, whenself.__wrappedis not set,deepcopyfails with exceptionRecursionError: maximum recursion depth exceeded.To repro, use this simple script:
Or