-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
pdb: import pdbcls lazily #5307
Conversation
] | ||
) | ||
assert result.ret == 4 | ||
assert result.ret == 1 |
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.
Note that this delays the UsageError now, which is not handled too well by pytest (e.g. via exitcode).
IIRC UsageErrors are only handled during config parsing/setup.
Codecov Report
@@ Coverage Diff @@
## master #5307 +/- ##
=========================================
+ Coverage 90.48% 91.3% +0.81%
=========================================
Files 115 115
Lines 26258 26260 +2
Branches 2587 2588 +1
=========================================
+ Hits 23760 23977 +217
+ Misses 2159 1952 -207
+ Partials 339 331 -8
Continue to review full report at Codecov.
|
class PytestPdbWrapper(cls._pdb_cls, object): | ||
pdb_cls = cls._import_pdb_cls() | ||
|
||
class PytestPdbWrapper(pdb_cls, object): |
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.
@asottile @nicoddemus
I'd also like to move the PytestPdbWrapper class out of the method, likely into a method that gets it cached based on pdb_cls
then (typically only one cache entry then).
This is good for less overhead in general, but allows for testing if the returned instance is the same as before (useful for pdbpp's global pdb handling).
This should be done before 5.0 to avoid conflicts in this area.
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.
Sure, sounds reasonable. I suspect this started as a small class so it was put inside the method for tersiness, but it has grown quite a lot since then.
Please do it right after this gets merged. 👍
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.
After #5319.
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.
Great job!
class PytestPdbWrapper(cls._pdb_cls, object): | ||
pdb_cls = cls._import_pdb_cls() | ||
|
||
class PytestPdbWrapper(pdb_cls, object): |
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.
Sure, sounds reasonable. I suspect this started as a small class so it was put inside the method for tersiness, but it has grown quite a lot since then.
Please do it right after this gets merged. 👍
Fixes #2064.