-
Notifications
You must be signed in to change notification settings - Fork 4
fix: accept message string in error and warning constructors #272
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #272 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 19 19
Lines 771 771
Branches 83 83
=======================================
Hits 759 759
Misses 10 10
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RedundantVerifyWarning
constructor
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.
Thanks for getting this issue resolved! Since we have multiple Warning and Exception classes with overloaded __init__
methods, I like your idea of a more robust fix, since good pytest-xdist support is important to me.
Can you apply the following suggestion to all our Warning and Exception classes that have custom __init__
methods defined? This will (hopefully obviously) also require updating some call sites
f"\t{stringify_call(rehearsal)}", | ||
"See https://michael.cousins.io/decoy/usage/errors-and-warnings/#redundantverifywarning", | ||
] | ||
def __init__(self, rehearsal: Union[VerifyRehearsal, str]) -> None: |
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.
suggestion: rather than overload the constructor, let's make a larger change:
- Add a
create
classmethod with the same signature as the old constructor, taking the rehearsal - Remove
__init__
, allowing it to use the default warning constructor
That way, our creation and (de)serialization both use the same string-based constructor, while message generation logic that relies on internal interfaces is split out to our own bespoke method
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.
@mcous The constructors are currently being used to set properties on the warning classes that appear to be used for testing, should I omit that? If I don't, the constructor can't take the standard parameters
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.
Oh right, good catch. The fact that they're relied upon in tests is an oversight and something I'm not particularly worried about maintaining. Use your best judgement to update tests if needed and I can fix anything up prior to merge.
The thing I am concerned about: removing those properties would technically be a breaking change. I am removing them in the next major version, so does something a little silly like this work to keep them for now?
class SomeWarning():
@classmethod
def create(cls, calls: ...):
message = ...
result = cls(message)
result.calls = calls
...
return result
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.
Oh also let me know if this is more effort than you want to / are available to take on! I really appreciate the work you've already done to observe and identify the issue
RedundantVerifyWarning
constructor
fixes #271