-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix(isUUID): handle of null version value #1777
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1777 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 102 102
Lines 2029 2029
Branches 457 457
=========================================
Hits 2029 2029
Continue to review full report at Codecov.
|
837cdb5
to
1c84bc2
Compare
033b22f
to
53999d5
Compare
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.
Thank you @theteladras for your efforts ! LGTM 🎉
We should replace this with the nullish coalescing operator when the CI/Testing problems will be fixed!
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 for your contrib! 🎉
The issue here lies in that, where the defaulting of argument is only managed for when the value of the version is 'undefined', and for other falsy values (in my case 'null') it accepts them. There are cases where functions do trigger callbacks with null values instead of undefined, and it usually leads to unexpected outcomes. I do believe that this is the general understanding of how the function should be used, when the version argument is falsy, it should check for 'all' pattern.
The particular issue is here:
Checklist