jquery / contribute.jquery.org

Developer documentation common to jQuery projects
https://contribute.jquery.org
Other
25 stars 76 forks source link

Style Guide: Context-changing methods as first method call #107

Open scottgonzalez opened 9 years ago

scottgonzalez commented 9 years ago

From https://github.com/jquery/jquery-ui/pull/1508#issuecomment-86893279:

We can make the rules for chained method calls a bit more lenient, something like "Context changing method calls at the start of the chain are acceptable on the first line.", along with an example, e.g. the filter/attr chain.

Example:

this.tabs.filter( function() {
    return $( this ).attr( "tabIndex" ) === 0;
} )
    .attr( "tabIndex", -1 );

Any arguments against this?

rxaviers commented 9 years ago

I dislike it. It seems like something is missing. It seems like someone just forgot to indent it.

My vote goes for either @dmethvin suggestion A or @scottgonzalez suggestion B.

A:

this.tabs.filter( function() {
  return $( this ).attr( "tabIndex" ) === 0;
} ).attr( "tabIndex", -1 );

B:

this.tabs.filter( function() {
        return $( this ).attr( "tabIndex" ) === 0;
    } )
    .attr( "tabIndex", -1 );

PS: The spacy } ) is really awkward. I couldn't get used to it yet :see_no_evil:.

scottgonzalez commented 9 years ago

A is definitely not in line with our style guide. B is actually what I expected. See the other thread for @jzaefferer's explanation of why he didn't want the extra indentation.