-
-
Notifications
You must be signed in to change notification settings - Fork 4.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
Add check for property #7421
Add check for property #7421
Conversation
@jdposthuma Thanks for the contribution. Can you go though the ToDo list? Add a test case, update the change log, etc? |
@dplewis - Done. I've added to the changelog. None of the other items seemed valid:
I haven't been able to capture (or simulate) the failing class
Too insignificant to add to docs
Doesn't change security
No new parse error code |
Except for the test case, you can delete the non-applicable TODOs from the list. The change makes sense, even if there wasn't a reported issue, just for code quality. However, it would be good to have a test case, to make your fix sustainable and prevent a future change from reverting your fix by mistake. And to verify that it solved the issue. Maybe you can give it another try? I classified this as bug of low severity due to the missing test case verification. |
Codecov Report
@@ Coverage Diff @@
## master #7421 +/- ##
==========================================
- Coverage 93.92% 93.92% -0.01%
==========================================
Files 181 181
Lines 13245 13247 +2
==========================================
+ Hits 12441 12442 +1
- Misses 804 805 +1
Continue to review full report at Codecov.
|
@jdposthuma Can you please rebase on master? |
Done |
@mtrezza - Test case added. I'm really glad you pushed for it because my original fix had a logic problem. 🙏 |
Hi @mtrezza - do I need to do anything else to get this to merge? |
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.
Just the changelog entry
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.
Looks good to me, thanks for this PR!
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.
LGTM!
Thanks guys! When's the next release planned? |
See #7271 (comment) |
🎉 This change has been released in version 5.0.0-beta.1 |
🎉 This change has been released in version 5.0.0 |
New Pull Request Checklist
Issue Description
This PR fixes the open issue.
Related issue: #7416
Approach
Simply added a truthy check
TODOs before merging