-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Build: Update eslint config and fix associated errors #3233
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,31 @@ define( [ | |
| var rbrace = /^(?:\{[\w\W]*\}|\[[\w\W]*\])$/, | ||
| rmultiDash = /[A-Z]/g; | ||
|
|
||
| function getData( data ) { | ||
| if ( data === "true" ) { | ||
| return true; | ||
| } | ||
|
|
||
| if ( data === "false" ) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( data === "null" ) { | ||
| return null; | ||
| } | ||
|
|
||
| // Only convert to a number if it doesn't change the string | ||
| if ( data === +data + "" ) { | ||
| return +data; | ||
| } | ||
|
|
||
| if ( rbrace.test( data ) ) { | ||
| return JSON.parse( data ); | ||
| } | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
| function dataAttr( elem, key, data ) { | ||
| var name; | ||
|
|
||
|
|
@@ -31,14 +56,7 @@ function dataAttr( elem, key, data ) { | |
|
|
||
| if ( typeof data === "string" ) { | ||
| try { | ||
| data = data === "true" ? true : | ||
| data === "false" ? false : | ||
| data === "null" ? null : | ||
|
|
||
| // Only convert to a number if it doesn't change the string | ||
| +data + "" === data ? +data : | ||
| rbrace.test( data ) ? JSON.parse( data ) : | ||
| data; | ||
| data = getData( data ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the size difference after extracting this to a function?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pull costs 32 bytes, not sure about this specific case
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems every other change should be optimizable by UglifyJS so 32 bytes seems to be quite a lot...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it weird that I find the ternary more readable?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mgol
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should consider disabling |
||
| } catch ( e ) {} | ||
|
|
||
| // Make sure we set the data so it isn't changed later | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -633,7 +633,19 @@ jQuery.each( { | |
|
|
||
| // Add which for click: 1 === left; 2 === middle; 3 === right | ||
| if ( !event.which && button !== undefined && rmouseEvent.test( event.type ) ) { | ||
| return ( button & 1 ? 1 : ( button & 2 ? 3 : ( button & 4 ? 2 : 0 ) ) ); | ||
| if ( button & 1 ) { | ||
| return 1; | ||
| } | ||
|
|
||
| if ( button & 2 ) { | ||
| return 3; | ||
| } | ||
|
|
||
| if ( button & 4 ) { | ||
| return 2; | ||
| } | ||
|
|
||
| return 0; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yet, I find these if statements much more readable than that ternary.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not everything could be generalized, but in general nested or in this case deep nested ternaries are less readable. For me thought, this is much more clearer
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if you have objections, it would be better to get them from the start - jquery/eslint-config-jquery#2
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sometimes it's hard to know if you like something or not until you see how it affects the code though. I like this rewritten code because it's easy to see the returns and know nothing below it matters for that case. With the ternaries it's much harder (for me at least) to figure that out, even when split across lines.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeeeah, i guess i get that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was looking into this further. I think this code was only for IE6-8. I noticed that this fallback expects quirksmode behavior (http://www.quirksmode.org/js/events_properties.html#button), not the standard behavior of In other words, if the reason for leaving this in was that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, awesome! Let's consider this in the next pr though, let's kill that baby :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dmethvin I'm not sure you understood what I meant. Note that the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh, was about do open ticket about this find and then there is Dave - #3235 :) |
||
| } | ||
|
|
||
| return event.which; | ||
|
|
||
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.
👍