-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Refactor assertString: Faster, less nested and more consistent. #2372
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
Conversation
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.
I think the tests are failing because we currently do not throw an error for ''
(an empty string).
Also; can you commit the unit tests as well?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2372 +/- ##
==========================================
- Coverage 99.95% 99.95% -0.01%
==========================================
Files 107 107
Lines 2454 2445 -9
Branches 619 617 -2
==========================================
- Hits 2453 2444 -9
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi, @WikiRik! |
codecov/project will be fixed with #2341 |
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 looks good to me
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
when you pass literally
because it is not caught by the first check. Should we explicitly check for null there as well? |
Hello, @pano9000! |
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.
should be ok now :-)
(however my approval is rather useless, as I am not a maintainer)
@profnandaa,
I have a possible improvement for the assertString.
I am reducing the number of necessary validations and also a more assertive approach, in my point of view (if this approach doesn't make sense, I would appreciate feedback on it, if possible).
The performance has also improved, it wasn't something incredible, but it was considerable.
Unit tests:
