Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Update jscs #10772

Closed
wants to merge 5 commits into from
Closed

Update jscs #10772

wants to merge 5 commits into from

Conversation

hzoo
Copy link
Contributor

@hzoo hzoo commented Jan 16, 2015

Updates jscs to latest (1.10.0) by updating grunt-jscs to 1.2.0

add rule disallowKeywordsOnNewLine: "else" ~ 10 changes

// Valid
if (x) {
} else {
}
// Invalid
if (x) {
}
else {
}

add rule disallowSpacesInCallExpression ~ 19 changes

// Valid
functionCall()
// Invalid
functionCall ()

add rule requireSpacesInForStatement ~ 1 change

// Valid
for (var i=0; i < length; i++)
// Invalid
for (var i=0;i < length;i++)
for (var i=0;  i < length;  i++)

add rule requireSpaceBeforeKeywords ~ 0 changes

// Valid
} else {
// Invalid
}else {

Others that could be added:
~32 changes: "requireCamelCaseOrUpperCaseIdentifiers": "ignoreProperties",
"requireSpaceBetweenArguments": true,
"disallowMultipleLineBreaks": true

@caitp

@caitp
Copy link
Contributor

caitp commented Jan 23, 2015

hmmm, not sure how i feel about making these rules even more strict. @petebacondarwin make the call on this one

@caitp caitp added this to the 1.4.x milestone Jan 23, 2015
@petebacondarwin
Copy link
Contributor

@hzoo - Thanks for the update.
This seems weird that updating broke a load of the code validation without changing the rules.
For instance my understanding was that validateParameterSeparator : ", " allowed any whitespace after the comma. Even the error message indicates that this is not working properly, when you have:

Missing space before function parameter '$q' at src/ngMock/angular-mocks.js :
   456 |angular.mock.$IntervalProvider = function() {
   457 |  this.$get = ['$browser', '$rootScope', '$q', '$$q',
   458 |       function($browser,   $rootScope,   $q,   $$q) {
-----------------------------------------------^

Can we look into the causes of this before we do this update?

@hzoo
Copy link
Contributor Author

hzoo commented Jan 23, 2015

Oh it got updated to only allow a single space. Actually I realize I added that change... jscs-dev/node-jscs#701. Now the rule enforces the value in quotes so it has to be exactly ", " which is strict. At the time I thought it was ok to enforce consistency since there isn't an align parameters style. (The error message wasn't updated either, so I'l have to change that as well).

EDIT: made the change in jscs for a future release jscs-dev/node-jscs#950

There will need to be an option to specify > 1 space to allow the code to have aligned parameters if we want that.

We can temporarily remove the validateParameterSeparator until another option is available if we want to keep the multiple spaces?

And can update to remove some rules - I suggested these rules since there weren't too many changes but of course I don't want to make it too strict so it's annoying to contribute.


Also we can probably remove .jscs.json.todo now - not sure if we want any of those rules still.

requireCurlyBraces is for single lines
disallowImplicitTypeConversion prevents '' + variable but i'm sure most people like that.
disallowMultipleLineBreaks - I think we have 2 line breaks for some tests?
validateJSDoc already discussed in the other PR, depends.

@petebacondarwin
Copy link
Contributor

Also we can probably remove .jscs.json.todo now - not sure if we want any of those rules still.

requireCurlyBraces is for single lines
disallowImplicitTypeConversion prevents '' + variable but i'm sure most people like that.
disallowMultipleLineBreaks - I think we have 2 line breaks for some tests?
validateJSDoc already discussed in the other PR, depends.

Yes, I agree that we don't these rules

@petebacondarwin
Copy link
Contributor

OK, so I am going to merge this - but turn off the validateParameterSeparator

@hzoo
Copy link
Contributor Author

hzoo commented Jan 30, 2015

Alright that sounds good - should I make the changes and rebase or you got it? I would fixup the first commit and then remove the jscs.json.todo.

@petebacondarwin
Copy link
Contributor

I am doing it now. Thanks

petebacondarwin pushed a commit that referenced this pull request Jan 30, 2015
petebacondarwin pushed a commit that referenced this pull request Jan 30, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants