-
-
Notifications
You must be signed in to change notification settings - Fork 698
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
Adding operator attribute to assertion error #1257
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1257 +/- ##
=========================================
+ Coverage 94.51% 94.6% +0.09%
=========================================
Files 32 33 +1
Lines 1677 1705 +28
Branches 405 415 +10
=========================================
+ Hits 1585 1613 +28
Misses 92 92
Continue to review full report at Codecov.
|
closing till I add tests. |
@keithamus , |
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.
Hi, @rpgeeganage, 😊
Thanks for the amazing PR, the code looks really good. I just pointed out some details that I think are worth correcting, but I'd love to get this in. Please let me know if I missed something or if there's anything else I can help you with.
Best,
Lucas
@lucasfcosta, |
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.
This LGTM, thank you very much 😊!
Can someone from @chaijs/core give this a last pass?
Is there something plugin authors who add assertions to the core library (yes I am biased lol) will need to do to support this? If so, does not updating plugins break something, or does it degrade gracefully? Cheers! 🙏 |
@astorije My hope is that this degrades gracefully, but if you want to support it in your plugin you can set via |
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.
Nice job!
I think it would be valuable to have steps listed in the release notes on 1) how to set this in your plugins if necessary and 2) how to check that it worked. Authors may not be familiar with Jest, so being able to correctly assess if that did it or if that's even required at all would be helpful I think. Either way, nice addition! 🙏 |
Hi everyone, |
Hello @rpgeeganage, I don't think there's any change to the code which is necessary, but as @astorije maybe worth adding information for plugin developers explaining how to use this flag. Since that can go in the release notes as @astorije recommended I think it's enough to add a brief explanation in the commit body so that we can include it in the release. I'm happy to get this in as is and the explanation in the commit's body would be very nice. |
@lucasfcosta , |
Hi @rpgeeganage, sorry I think my comment was a bit confusing. Just add it to the body of the commit and we will add it to the release notes when creating the release. A |
@lucasfcosta , |
This is an attempt to add
operator
property for theAssertionError
.(As per the issue #1252)
Operators are as follows.
Please give me feedback on this draft PR.
Thank you.