Skip to content

capture: factor out _get_multicapture #6788

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

Merged
merged 3 commits into from
Feb 22, 2020

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Feb 21, 2020

Ref: #6671 (comment)

TODO:

@@ -66,6 +66,18 @@ def pytest_load_initial_conftests(early_config: Config):
sys.stderr.write(err)


def _get_multicapture(method: str) -> "MultiCapture":
Copy link
Member

Choose a reason for hiding this comment

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

TIOLI: Use Literal["fd", "sys", "no", "tee-sys"] instead of str.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, here I took your advice that it should not be used when the API (even if internal IIUC) is going to change (when adding new methods).
But I would prefer the Literal also, I'm just confused.. :)

Copy link
Member

Choose a reason for hiding this comment

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

Hehe :) Well up to now we discussed return types, but this is an input type. Input types should be as restrictive as possible to rule out illegal inputs.

This is assuming the function is not a validating function (whose purpose is to take out general inputs and check them). In this case the input is already validated by the --capture arg parsing (which has choices=["fd", "sys", "no", "tee-sys"]), so any other kind of input is a not supposed to reach here.

If you choose to make this a Literal, it would make sense to also change CaptureManager(method=...) to have this type already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

But it might become a validating / more flexible method (https://github.com/pytest-dev/pytest/pull/6671/files#diff-e4269384a671a462a4824e8b06d4644bR97-R121), where choices then however would need to be factored out likely. But until then I think it makes sense to use Literal here already.

@blueyed blueyed merged commit 706ea86 into pytest-dev:master Feb 22, 2020
@blueyed blueyed deleted the capture-refactor-2 branch February 22, 2020 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants