Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ESLint #13549

Merged
merged 5 commits into from Jan 4, 2017
Merged

ESLint #13549

merged 5 commits into from Jan 4, 2017

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented May 23, 2016 •
edited

This PR adds ESLint code checking. JSHint/JSCS are not removed yet and ESLint is only part of the mocha test suite so far until emberjs-build supports ESLint too. Most of the JSHint/JSCS rule equivalents have been enabled and only two of the custom JSCS rules are missing at the moment and need to be reimplemented on the ESLint API.

partly resolves #13006

  • port require-comments-to-include-access rule
  • port disallow-const-outside-module-scope rule

/cc @stefanpenner @rwjblue

@homu
Copy link
Contributor

homu commented May 24, 2016

☔ The latest upstream changes (presumably #13542) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87 Turbo87 force-pushed the eslint branch 2 times, most recently from bf4133a to ca20f1a Compare May 24, 2016 14:24
@Turbo87
Copy link
Member Author

Turbo87 commented May 24, 2016

All JSHint/JSCS equivalents have been ported and/or enabled now! 🎉

@Turbo87
Copy link
Member Author

Turbo87 commented May 24, 2016

looks like I'm being bitten by the node_modules caching on TravisCI... :-/

@Turbo87 Turbo87 force-pushed the eslint branch 2 times, most recently from aefce60 to 3343e3b Compare May 25, 2016 14:54
@Turbo87
Copy link
Member Author

Turbo87 commented May 25, 2016

I've extracted the custom ESLint rules into https://github.com/Turbo87/eslint-plugin-ember-internal to avoid the subproject issues that caused the previous CI failures

@homu
Copy link
Contributor

homu commented May 25, 2016

☔ The latest upstream changes (presumably b8c5e08) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87 Turbo87 force-pushed the eslint branch 2 times, most recently from eafe29f to 4b0d84d Compare May 25, 2016 21:39
@rwjblue
Copy link
Member

rwjblue commented May 26, 2016

Why are there so many file changes here? I believe that we should be applying the existing rules only...

@Turbo87
Copy link
Member Author

Turbo87 commented May 26, 2016

@rwjblue except for the last commit: because JSHint apparently isn't that great at catching these things. It's been the same in all the other repos I've converted so far. ESLint usually found a lot more issues than JSHint.

@homu
Copy link
Contributor

homu commented May 26, 2016

☔ The latest upstream changes (presumably ee55266) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87 Turbo87 mentioned this pull request May 28, 2016
Closed
stefanpenner
stefanpenner reviewed Jun 3, 2016
View reviewed changes
packages/ember-glimmer/tests/integration/helpers/closure-action-test.js
this._super(...arguments);
outerComponent = this;
},
// init() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still WIP?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here was that outerComponent was unused. I wasn't sure if it was actually supposed to be used in a test somewhere so I commented it out for now. Let me know if I should remove is completely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the tests pass without it, just delete it. We can always get it back if needed 😸

@stefanpenner
Copy link
Member

stefanpenner commented Jun 3, 2016 •
edited

I'm 👍 on this, all changes look good. I did point out some what appears to be WIP

@rwjblue
Copy link
Member

rwjblue commented Jun 3, 2016

Please squash

@rwjblue
Copy link
Member

rwjblue commented Jun 3, 2016

ESLint is only part of the mocha test suite so far until emberjs-build supports ESLint too

This must be fixed before landing. The linting failures need to be included in the normal ember test --server output (just like in an ember-cli app).

@Turbo87
Copy link
Member Author

Turbo87 commented Jun 3, 2016

@rwjblue @stefanpenner would love to get some feedback on emberjs/emberjs-build#161, then I'll start working on integrating ESLint into ember test --server.

@homu
Copy link
Contributor

homu commented Jun 4, 2016

☔ The latest upstream changes (presumably #13553) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87 Turbo87 mentioned this pull request Jun 5, 2016
Merged
@Turbo87 Turbo87 mentioned this pull request Jun 13, 2016
Merged
@homu
Copy link
Contributor

homu commented Jul 1, 2016

☔ The latest upstream changes (presumably #13775) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87 Turbo87 force-pushed the eslint branch 2 times, most recently from 2134c2c to 6aea607 Compare July 1, 2016 20:39
@homu
Copy link
Contributor

homu commented Jul 19, 2016

☔ The latest upstream changes (presumably #13838) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87 Turbo87 mentioned this pull request Nov 11, 2016
Merged
@homu
Copy link
Contributor

homu commented Nov 30, 2016

☔ The latest upstream changes (presumably #14658) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Copy link
Contributor

homu commented Jan 3, 2017

☔ The latest upstream changes (presumably #14776) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87
Copy link
Member Author

Turbo87 commented Jan 4, 2017

@rwjblue rebased and updated emberjs-build to v0.19.0

@rwjblue
Copy link
Member

rwjblue commented Jan 4, 2017

Restarted travis build (was failing bizarrely)

@Turbo87
Copy link
Member Author

Turbo87 commented Jan 4, 2017

@rwjblue failure might be related to the fact that I didn't update the yarn lockfile

(and I still believe that it's stupid that it's failing because of that...)

@Turbo87
Copy link
Member Author

Turbo87 commented Jan 4, 2017

🍏

@rwjblue rwjblue merged commit 1737044 into emberjs:master Jan 4, 2017
@rwjblue
Copy link
Member

rwjblue commented Jan 4, 2017

Thanks for working so hard on this!

@homu homu mentioned this pull request Jan 4, 2017
Merged
@Turbo87 Turbo87 deleted the eslint branch January 4, 2017 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace JSHint/JSCS with ESLint
4 participants
@Turbo87 @homu @rwjblue @stefanpenner