-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
replace wierd syntax with clear expression #4814
replace wierd syntax with clear expression #4814
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.
Thank you. The examples are typically in the style of who contributed them. I suggest though using a different API than .includes so we can merge into the 4.x branch (see CI failures).
FYI, you can ignore the CI failures; a transient dependency just published a version that broke it; I'll get that addressed when I get home and otherwise your PR I think is good to go. |
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.
Neat 👍
9bb1a59
to
bb26325
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 for taking the time to help improve our examples!
The use of
!~
hides the completely understandableapiKeys.indexOf(key) < 0
orif(!apiKeys.includes(key))
.Since this is example code, this should be crisp and clean.
I don't see any advantage or improvement in the use of a "bitwise negation of -1 is 0 and thus falsy, so I can use a boolean not on it for my if statement" to replace a straight "result is less than 0". I even would consider it is slower than a simple compare, since it's two operations. The newer API (includes) is even more literate in coding style.
I suggest this is fixed.