Add jscs rule for function spacing by hzoo · Pull Request #9708 · angular/angular.js

hzoo · 2014-10-21T02:08:35Z

Add rule disallowSpacesInFunctionDeclaration beforeOpeningRoundBrace.

function a() {

};

Add rule disallowSpacesInNamedFunctionExpression beforeOpeningRoundBrace.

var a = function a() {

}

Add rule requireSpacesInFunction beforeOpeningCurlyBrace. Applies to all types of functions.

Didn't include spacing for round brace for anonymous functions yet.
Checking only anonymous function expressions: for function (e), got 1121 times.
Checking only anonymous function expressions: for function(e), got 9159 times.

cc: @caitp @petebacondarwin

caitp · 2014-10-21T02:20:38Z

LGTM --- although I'd still like the function () vs function() inconsistency fixed :< can wait for another CL.

hzoo · 2014-10-21T21:16:49Z

Ok added a new commit for 'function()`. That was a lot haha. @caitp

caitp · 2014-10-21T21:34:18Z

wunderbar~

caitp · 2014-10-21T21:35:16Z

Hah, changing the generated locale files... did you also change the JS in the locale files generator scripts? (i cant tell because the diff is too big)

hzoo · 2014-10-21T21:49:24Z

Not sure which ones are the generator scripts? src/ng/locale.js?, or everything in i18n/?

caitp · 2014-10-21T21:51:11Z

under the i18n directory and subdirectories (stuff like this https://github.com/angular/angular.js/blob/master/i18n/src/closureI18nExtractor.js#L173-L197)

hzoo · 2014-10-21T21:52:58Z

Ok I'l have to do that since grunt jscs only runs under src/ and test/.

In the Gruntfile, src: ['src/**/*.js', 'test/**/*.js']

So we'll probably want to add i18n/ in and maybe some other stuff?

Although after doing that, there will probably be a lot of other changes that have to be made since jscs was never run on that folder.

caitp · 2014-10-21T21:54:52Z

well it's generated code, so most of it is within string literals. The problem is that it will overwrite all the changes to src/ngLocale that you did when it's run again, and those files will start failing the linting

hzoo · 2014-10-21T21:56:40Z

Oh I see what you mean.

hzoo · 2014-10-21T22:00:00Z

So I'l remove all changes to any src/ngLocale files.
Then look through the string literals / fix the styling there and keep testing till the generated ngLocale files pass?

caitp · 2014-10-21T22:03:33Z

yeah pretty much

hzoo · 2014-10-21T23:38:01Z

Hmm looks like I'm having some problems with running the generator. Not sure if i'm doing something weird.

I tried running ./i18n/generate.sh and got:

Created directory "../src/ngLocale/"
Processing currency and number symbols ...
Processing datetime symbols ...

angular.js/node_modules/q/q.js:126
throw e;
^
Error: ENOENT, open 'angular.js/i18n/src/../closure/datetimeSymbols.js'

But yeah, only one style change is needed for the generated files at line 113: "pluralCat": function (n, opt_precision)

looks like L89: var temp = goog.i18n.pluralRules.select.toString(). creates:

function (n) {
  if (n >= 0 && n < 2) {
    return goog.i18n.pluralRules.Keyword.ONE;
  }
  return goog.i18n.pluralRules.Keyword.OTHER;
}

which has that space (I guess that's the toString() default)

even though all the functions in i18n/closure/pluralRules are function(n) and not function (n)

caitp · 2014-10-22T01:58:10Z

it looks like the file is in the tree as datetimesymbols.js rather than datetimeSymbols.js --- I guess we all have case-insensitive file systems for some reason :x (apparently my mac's fs is case insensitive currently. huh.) --- easy patch to write, and that's worth fixing anyways. Why not change that over in the PR in a separate commit? I'll give it an LGTM

hzoo · 2014-10-22T02:40:17Z

Ok renamed it and generate.sh ran!

I added replace('function (n','function(n'); to the string which fixed it for all the generated files except for

src/ngLocale/angular-locale_en-dsrt-us.js
src/ngLocale/angular-locale_en-dsrt.js
src/ngLocale/angular-locale_ms-bn.js
src/ngLocale/angular-locale_ms-my.js

and not sure why..

looking at https://github.com/angular/angular.js/tree/master/src/ngLocale - it looks like those 4 aren't generated based on the last commit that regenerated the locale files?

gkalpak · 2014-10-22T05:30:41Z

Shouldn't there be a space between arguments ? ;)

Haha yeah good catch. I think that the validateParameterSeparator rule seems to only check for function (a, b, c) { and we need to use disallowSpaceBeforeBinaryOperators and requireSpaceAfterBinaryOperators with , to check for spaces between arguments for actual function calls like 'asdf'.replace(a, b).

Although should I be running jscs on the other files too? Since now it's only checking files under 'src' and 'test'

hzoo · 2014-10-22T12:57:00Z

I pushed a commit that fixed the js in the generated scripts except the 4 mentioned above so I did those manually. Still need to figure out why those 4 aren't being created and why the build is failing though.

caitp · 2014-10-22T17:58:27Z

the build failure looks like a flake to me

caitp · 2014-10-22T17:59:30Z

if those files aren't being created, it's possible the closure build is out of date, or maybe some locales were removed from closure. run the closure update script and maybe clear out src/ngLocale's js files

hzoo · 2014-10-22T18:19:30Z

Ok, after clearing src/ngLocale and running update-closure.sh, closure/currencySymbols.js was updated but the 4 locales aren't updated (still have the function ()).

Yeah the last time angular-locale_en-dsrt-us.js, angular-locale_en-dsrt.js,
angular-locale_ms-bn.js,angular-locale_ms-my.js were updated were ~ a year ago while the other files were a month ago.

Also there aren't references to those locales in the /closure/datetimeSymbolsExt.js and /closure/numberSymbolsExt.js files unlike the rest. So I guess like you said they were removed or weren't added in?

caitp · 2014-10-22T18:36:50Z

@eltacodeldiablo see caitp#1 --- when I run with those fixes (some of them are just extra) and do grep 'function (' -r src/ngLocale, I get no output. So it should work

caitp · 2014-10-22T18:37:17Z

err -- I think I opened that PR wrong lol. Hang on.

caitp · 2014-10-22T18:37:47Z

https://github.com/eltacodeldiablo/angular.js/pull/1 there you go

hzoo · 2014-10-22T22:59:06Z

From this: 0eb2c2a#diff-4c51149c7932e91d0b490a8427472ab6L2706

it looks like those 4 files are being used anymore so I removed them.

@caitp

…undBrace

…oundBrace

This rule enforces a space after the curly brace for function declarations, anonymous function expressions, and named function expressions.

…OpeningRoundBrace, including i18n generator

hzoo · 2014-10-23T11:25:09Z

Oh ok I was able to rebase on top of master (also amended the commit message since it was the wrong rule to AnonymousFunctionExpression)

hzoo · 2014-10-23T19:47:41Z

@caitp ah sorry accidentally closed

caitp · 2014-10-23T19:52:39Z

it looks like those 4 files are being used anymore so I removed them.

Hmm --- so I guess if we aren't claiming to support those anymore, it's fine to get rid of them... but I feel like people will file bugs about that :(

Well, lets worry about it later, I guess.

caitp · 2014-10-23T19:53:47Z

Looks good to me, although I'd rather the re-generated files were in a separate commit than the jscs fix

hzoo · 2014-10-23T19:55:02Z

ok so you want the locale file changes in a separate commit? What about the ones in /i18n? Yeah doing a lot of different stuff this commit like updating i18n/closure/currencySymbols.js
and renaming i18n/closure/{datetimesymbols.js → datetimeSymbols.js}

hzoo · 2014-10-23T20:01:46Z

Well at least for ms-bn it was removed in commit 6382e21 so I don't know how it came back.

For en-dsrt-us it looks like another commit replaced it with en_dg
For ms-my, ms_Latn_MY

caitp · 2014-10-23T20:02:11Z

Closed by 40bbc98..d3b1f50

caitp · 2014-10-23T20:03:50Z

hopefully nobody wants to revert these arbitrary style rules we've added ;-;

hzoo · 2014-10-23T20:11:09Z

xp I think the last one for anonymous functions might be annoying to some folk. Although the code was 90% function() for anonymous.

caitp · 2014-10-23T20:12:34Z

they will learn to deal with it :>

mary-poppins added cla: yes and removed cla: no labels Oct 21, 2014

caitp reviewed Oct 21, 2014
View reviewed changes

rodyhaddad added needs: review type: refactor labels Oct 21, 2014

rodyhaddad added this to the 1.3.x milestone Oct 21, 2014

gkalpak reviewed Oct 22, 2014
View reviewed changes

hzoo added 2 commits October 23, 2014 07:20

style(*): add disallowSpacesInNamedFunctionExpression beforeOpeningRo…

7db03b8

…undBrace

style(*): add rule disallowSpacesInFunctionDeclaration beforeOpeningR…

2110069

…oundBrace

hzoo added 3 commits October 23, 2014 07:20

style(*): add rule requireSpacesInFunction beforeOpeningCurlyBrace

1d677c0

This rule enforces a space after the curly brace for function declarations, anonymous function expressions, and named function expressions.

fix(i18n): rename datetimeSymbols to be camelCase

961eb6f

style(*): add rule disallowSpacesInAnonymousFunctionExpression before…

d2d18f0

…OpeningRoundBrace, including i18n generator

hzoo closed this Oct 23, 2014

hzoo deleted the add-jscs-rule-function-spacing branch October 23, 2014 11:25

hzoo restored the add-jscs-rule-function-spacing branch October 23, 2014 11:25

hzoo reopened this Oct 23, 2014

caitp closed this Oct 23, 2014

hzoo deleted the add-jscs-rule-function-spacing branch October 25, 2014 03:51

Conversation

hzoo commented Oct 21, 2014

Uh oh!

caitp Oct 21, 2014

Choose a reason for hiding this comment

Uh oh!

hzoo commented Oct 21, 2014

Uh oh!

caitp commented Oct 21, 2014

Uh oh!

caitp commented Oct 21, 2014

Uh oh!

hzoo commented Oct 21, 2014

Uh oh!

caitp commented Oct 21, 2014

Uh oh!

hzoo commented Oct 21, 2014

Uh oh!

caitp commented Oct 21, 2014

Uh oh!

hzoo commented Oct 21, 2014

Uh oh!

hzoo commented Oct 21, 2014

Uh oh!

caitp commented Oct 21, 2014

Uh oh!

hzoo commented Oct 21, 2014

Uh oh!

caitp commented Oct 22, 2014

Uh oh!

hzoo commented Oct 22, 2014

Uh oh!

gkalpak Oct 22, 2014

Choose a reason for hiding this comment

Uh oh!

hzoo Oct 22, 2014

Choose a reason for hiding this comment

Uh oh!

hzoo commented Oct 22, 2014

Uh oh!

caitp commented Oct 22, 2014

Uh oh!

caitp commented Oct 22, 2014

Uh oh!

hzoo commented Oct 22, 2014

Uh oh!

caitp commented Oct 22, 2014

Uh oh!

caitp commented Oct 22, 2014

Uh oh!

caitp commented Oct 22, 2014

Uh oh!

hzoo commented Oct 22, 2014

Uh oh!

hzoo commented Oct 23, 2014

Uh oh!

hzoo commented Oct 23, 2014

Uh oh!

caitp commented Oct 23, 2014

Uh oh!

caitp commented Oct 23, 2014

Uh oh!

hzoo commented Oct 23, 2014

Uh oh!

hzoo commented Oct 23, 2014

Uh oh!

caitp commented Oct 23, 2014

Uh oh!

caitp commented Oct 23, 2014

Uh oh!

hzoo commented Oct 23, 2014

Uh oh!

caitp commented Oct 23, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development