Style updates based on latest version of style guide #1588
Conversation
|
I looked through ~50% of this and it seems good to land. |
| @@ -246,7 +257,7 @@ $.widget.bridge = function( name, object ) { | |||
| }; | |||
| }; | |||
|
|
|||
| $.Widget = function( /* options, element */ ) {}; | |||
| $.Widget = function( /* Options, element */ ) {}; | |||
There was a problem hiding this comment.
This one is kinda wrong. Won't hurt anyone, but still...
There was a problem hiding this comment.
Hmmm... I wonder if we can get JSCS to catch that scenario.
There was a problem hiding this comment.
@scottgonzalez It DID catch this which is why it became uppercase.
@markelog @mikesherov It seems like the requireCapitalizedComments is completely unenforceable in a realistic sense all of these valid comments fail with it
// http://google.com
// #3456 this does xxx
// to yyy so that zzz.
function( /* option */ ) {
/*
test( "i dont want this to run", function() {
*/There are probably more cases then just this, but because of these ones I was forced to disable this rule within jQuery UI ( though i believe i got most if not all the valid comments fixed before disabling )
I know there is the allExecpt option but that did not seem very useful here since it seems to do a whole word match so wont work on urls or tests like in the example above. Also for the multi line example would have to just add a ton of random words which could cause things to be missed elsewhere.
If you consider this a bug i will move it overto the JSCS tracker
There was a problem hiding this comment.
I meant I wonder if we can get JSCS to catch the scenario of an inline comment inside an argument list so that it doesn't apply the capitalization. But the other examples you've posted do make this seems very hard to enforce.
There was a problem hiding this comment.
@scottgonzalez just to be clear JSCS did not capitalize it I did, trying to get this passing. I meant to ( and did before landing ) revert them. JSCS only warns on first letter capitalization. The changes for this in the PR for comment capitalization were done with a search and replace. I was trying to get 100% passage with out having to have this rule exceptions from the jQuery preset. However after finding the examples above i decided this was not possible.
There was a problem hiding this comment.
( and did before landing )
I wonder, did you aware that you add spacing inside parentheses in English now? :-)
Yeah, requireCapitalizedComments requires some work - jscs-dev/node-jscs#1686 could you add cases you find in there?
I was trying to get 100% passage with out having to have this rule exceptions from the jQuery preset
I guess if rule mess things up instead of helping, we probably should consider removing it from the preset until it's working right
| $.fn.extend({ | ||
| effect: function( /* effect, options, speed, callback */ ) { | ||
| $.fn.extend( { | ||
| effect: function( /* Effect, options, speed, callback */ ) { |
There was a problem hiding this comment.
"same" here, as below for $.Widget.
Updates to follow latest version of style guide in all files.