Skip to content

Use attrs with all Repr classes #6739

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 8 commits into from
Mar 3, 2020
Merged

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Feb 14, 2020

Follow-up to #6732.

reprfuncargs = attr.ib(type=Optional["ReprFuncArgs"])
reprlocals = attr.ib(type=Optional["ReprLocals"])
# XXX/NOTE: changed arg name: filelocrepr => reprfileloc
reprfileloc = attr.ib(type=Optional["ReprFileLocation"])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume we can go with s/filelocrepr/reprfileloc, or should it support the old name? (it was at least used internally).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume we can go with s/filelocrepr/reprfileloc, or should it support the old name? (it was at least used internally).

Still an open question..

Copy link
Member

Choose a reason for hiding this comment

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

I think it is OK to change the parameter name and keep the attribute reprfileloc: I doubt people create ReprEntry explicitly (it is not possible to import ReprEntry through the public API).

@blueyed blueyed changed the title [WIP] Use attrs with all Repr classes Use attrs with all Repr classes Feb 19, 2020
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

LGTM, except the mixin class (I really dislike those!), which makes me wonder if there is particular reason you decided to break the ExceptionRepr subclassing? The following change on top of yours brings it back and seems to work fine:

diff --git a/src/_pytest/_code/code.py b/src/_pytest/_code/code.py
index 8c349c17e..2bf3b76fa 100644
--- a/src/_pytest/_code/code.py
+++ b/src/_pytest/_code/code.py
@@ -928,12 +928,8 @@ class TerminalRepr:
         raise NotImplementedError()
 
 
-class _SectionsRepr:
-    """Mixin for handling sections.
-
-    Only `ExceptionRepr` takes it in `__init__`, but others use it also.
-    """
-
+@attr.s
+class ExceptionRepr(TerminalRepr):
     def __attrs_post_init__(self):
         self.sections = []  # type: List[Tuple[str, str, str]]
 
@@ -947,12 +943,7 @@ class _SectionsRepr:
 
 
 @attr.s
-class ExceptionRepr(_SectionsRepr, TerminalRepr):
-    sections = attr.ib(type=List[Tuple[str, str, str]])
-
-
-@attr.s
-class ExceptionChainRepr(_SectionsRepr, TerminalRepr):
+class ExceptionChainRepr(ExceptionRepr):
     chain = attr.ib(
         type=Sequence[
             Tuple["ReprTraceback", Optional["ReprFileLocation"], Optional[str]]
@@ -976,7 +967,7 @@ class ExceptionChainRepr(_SectionsRepr, TerminalRepr):
 
 
 @attr.s
-class ReprExceptionInfo(_SectionsRepr, TerminalRepr):
+class ReprExceptionInfo(ExceptionRepr):
     reprtraceback = attr.ib(type="ReprTraceback")
     reprcrash = attr.ib(type="ReprFileLocation")
 

@blueyed
Copy link
Contributor Author

blueyed commented Feb 28, 2020

@bluetech
I've pushed your patch, thanks!
It was necessary before __attrs_post_init__ was used only I guess.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

reprfuncargs = attr.ib(type=Optional["ReprFuncArgs"])
reprlocals = attr.ib(type=Optional["ReprLocals"])
# XXX/NOTE: changed arg name: filelocrepr => reprfileloc
reprfileloc = attr.ib(type=Optional["ReprFileLocation"])
Copy link
Member

Choose a reason for hiding this comment

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

I think it is OK to change the parameter name and keep the attribute reprfileloc: I doubt people create ReprEntry explicitly (it is not possible to import ReprEntry through the public API).

@blueyed blueyed merged commit b11bfa1 into pytest-dev:master Mar 3, 2020
@blueyed blueyed deleted the all-attrs branch March 3, 2020 20:53
shawnbrown added a commit to shawnbrown/pytest-datatest that referenced this pull request Jul 22, 2020
This is done because ReprEntry's `filelocrepr` argument was
renamed to `reprfileloc`.

For details, see: pytest-dev/pytest#6739
blueyed added a commit to blueyed/pytest that referenced this pull request Nov 8, 2020
Co-authored-by: Ran Benita <[email protected]>

(cherry picked from commit b11bfa1)

Ref: pytest-dev#6739

Conflicts:
	src/_pytest/_code/code.py
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.

4 participants