Add jscs rules #9792

hzoo · 2014-10-25T15:11:23Z

WIP

Maybe we just exclude ngLocale/ from style check because they are generated anyway?

caitp · 2014-10-25T19:56:35Z

If you want to exclude ngLocale from jscs checks, I think that's okay --- but we definitely want to jshint it.

hzoo · 2014-10-26T01:09:46Z

What do you think about the other rules?
Do you think I should add another commit to the PR to exclude it or replace the last commit that changed all the locale files?

caitp · 2014-10-26T01:26:36Z

I'm not a fan of all of the changes --- most of it's fine.

One thing though, I think we should move jscs linting until after the tests run for the unit test job. I care a lot more about stuff working right than I care about it matching the style rules, which can usually be easily fixed during check-in.

hzoo · 2014-10-26T01:39:33Z

Yeah styles are just nice to have. So you mean changing the Gruntfile so
grunt.registerTask('test', 'Run unit, docs and e2e tests with Karma', ['jshint', 'jscs', 'package','test:unit','test:promises-aplus', 'tests:docs', 'test:protractor']);
becomes
grunt.registerTask('test', 'Run unit, docs and e2e tests with Karma', ['jshint', 'package','test:unit','jscs','test:promises-aplus', 'tests:docs', 'test:protractor']);

or moving it to the very end?

caitp · 2014-10-26T01:42:20Z

No, nothing in the grunt file, in the travis build script: https://github.com/angular/angular.js/blob/master/scripts/travis/build.sh#L8

We should move that to the last job, or maybe pre-docsapp tests

hzoo · 2014-10-26T01:50:09Z

Ok removed the other rule, reordered the tasks, excluded src/ngLocale from jscs check.

hzoo · 2014-10-26T18:54:28Z

I also noticed in the .editorconfig:

[src/ngLocale/**]
insert_final_newline = false

From #8278,
I removed the final newline requirement from src/ngLocale files because all of the 446 files in that directory are missing a final newline. I assume these are generated by a script/program that leaves out the final newline.

So I guess we can remove that if since the script add's the new line.

caitp · 2014-10-27T16:36:53Z

I feel like @IgorMinar should have some input on merging more of these style rules, otherwise I'm just injecting my own opinion into it. It works for me, so if it's cool with googlers I guess it can land

hzoo · 2014-10-27T17:07:54Z

Yeah and probably a look at the previous changes that weren't in the .jscs.json.todo

… generated script

…ripts

IgorMinar · 2014-10-31T13:55:55Z

alright. I don't see any red flags here. @caitp can you get this in please?

hzoo · 2014-10-31T15:33:00Z

Nice thanks!

caitp · 2014-10-31T15:34:10Z

✨ 👏 ✨

This was changed as part of angular#9792. While the logic is sound that style errors shouldn't block tests from running, ddescribe should run before the tests. Otherwise, when Travis exits with a warning after some browsers have run, ddescribe doesn't get run and it doesn't immediately become apparent that not all tests have run.

Now, before the tests run, ddescribe and jshint will run. After the tests, jscs will run. Merge conflicts should be caught by jshint, that's why this task has been removed. The order was orginally changed as part of angular#9792. While the logic is sound that style errors shouldn't block tests from running, ddescribe should run before the tests. Otherwise, when Travis exits with a warning after some browsers have run, ddescribe doesn't get run and it doesn't immediately become apparent that not all tests have run.

…e tests Now, before the tests run, ddescribe, merge-conflicts, and jshint will run. After the tests, jscs will run. The order was orginally changed as part of angular#9792. While the logic is sound that style errors shouldn't block tests from running, ddescribe should run before the tests. Otherwise, when Travis exits with a warning after some browsers have run, ddescribe doesn't get run and it doesn't immediately become apparent that not all tests have run.

Previously, ddescribe, merge-conflicts, jshint, and jscs would run after unit & e2e tests ran. The order was orginally changed as part of angular#9792. While the logic is sound that style errors shouldn't block tests from running, ddescribe should always run. This was not guaraneteed; when Travis exits with a warning after some browsers have run, ddescribe doesn't get run and it doesn't become apparent that not all tests have run. Additionally, a separate job clearly separates style from test errors, which e.g. means you can open a PR that includes an iit to speed up the job, and see immediately if the test passes, because the ddescribe error is in another job.

Previously, ddescribe, merge-conflicts, jshint, and jscs would run after unit & e2e tests ran. The order was orginally changed as part of #9792. While the logic is sound that style errors shouldn't block tests from running, ddescribe should always run. This was not guaraneteed; when Travis exits with a warning after some browsers have run, ddescribe doesn't get run and it doesn't become apparent that not all tests have run. Additionally, a separate job clearly separates style from test errors, which e.g. means you can open a PR that includes an iit to speed up the job, and see immediately if the test passes, because the ddescribe error is in another job.

Previously, ddescribe, merge-conflicts, jshint, and jscs would run after unit & e2e tests ran. The order was orginally changed as part of angular#9792. While the logic is sound that style errors shouldn't block tests from running, ddescribe should always run. This was not guaraneteed; when Travis exits with a warning after some browsers have run, ddescribe doesn't get run and it doesn't become apparent that not all tests have run. Additionally, a separate job clearly separates style from test errors, which e.g. means you can open a PR that includes an iit to speed up the job, and see immediately if the test passes, because the ddescribe error is in another job.

Previously, ddescribe, merge-conflicts, jshint, and jscs would run after unit & e2e tests ran. The order was orginally changed as part of #9792. While the logic is sound that style errors shouldn't block tests from running, ddescribe should always run. This was not guaraneteed; when Travis exits with a warning after some browsers have run, ddescribe doesn't get run and it doesn't become apparent that not all tests have run. Additionally, a separate job clearly separates style from test errors, which e.g. means you can open a PR that includes an iit to speed up the job, and see immediately if the test passes, because the ddescribe error is in another job.

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

hzoo force-pushed the add-jscs-rules branch from 59b4eab to 06b56ef Compare October 26, 2014 01:48

hzoo force-pushed the add-jscs-rules branch 2 times, most recently from 33f1ca0 to 81d1968 Compare October 26, 2014 02:19

hzoo and others added 10 commits October 30, 2014 10:27

style(*): add rule requireSpaceBeforeBlockStatements

ad72a9c

style(*): add operators to rule disallowSpaceAfterPrefixUnaryOperators

bcce03e

style(*): add rule disallowSpaceBeforePostfixUnaryOperators

6ee1a4a

style(*): add rule disallowSpacesInsideArrayBrackets

f7990d2

style(*): add rule requireCommaBeforeLineBreak

5fff71e

style(*): add rule validateLineBreaks

1244294

style(*): move rule requireLineFeedAtFileEnd from jscs todo, fix i18n…

0e6837e

… generated script

chore(travis): reorder linting tasks to after unit tests

6c41694

style(*): exclude generated files in src/ngLocale from jscs check

1bcaee6

chore(.editorconfig): remove settings for ngLocale after change to sc…

7c90ddf

…ripts

hzoo force-pushed the add-jscs-rules branch from 18c9f41 to 7c90ddf Compare October 30, 2014 14:28

IgorMinar added this to the 1.3.1 milestone Oct 31, 2014

IgorMinar assigned IgorMinar and caitp and unassigned IgorMinar Oct 31, 2014

caitp closed this in 030101a Oct 31, 2014

hzoo deleted the add-jscs-rules branch November 1, 2014 02:06

Narretz mentioned this pull request Dec 11, 2015

chore(travis/build.sh): move ddescribe check before unit tests #13517

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jscs rules #9792

Add jscs rules #9792

hzoo commented Oct 25, 2014

caitp commented Oct 25, 2014

hzoo commented Oct 26, 2014

caitp commented Oct 26, 2014

hzoo commented Oct 26, 2014

caitp commented Oct 26, 2014

hzoo commented Oct 26, 2014

hzoo commented Oct 26, 2014

caitp commented Oct 27, 2014

hzoo commented Oct 27, 2014

IgorMinar commented Oct 31, 2014

hzoo commented Oct 31, 2014

caitp commented Oct 31, 2014

Add jscs rules #9792

Add jscs rules #9792

Conversation

hzoo commented Oct 25, 2014

caitp commented Oct 25, 2014

hzoo commented Oct 26, 2014

caitp commented Oct 26, 2014

hzoo commented Oct 26, 2014

caitp commented Oct 26, 2014

hzoo commented Oct 26, 2014

hzoo commented Oct 26, 2014

caitp commented Oct 27, 2014

hzoo commented Oct 27, 2014

IgorMinar commented Oct 31, 2014

hzoo commented Oct 31, 2014

caitp commented Oct 31, 2014