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.

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

Eslint config defaults #526

Merged
merged 3 commits into from
Sep 27, 2015
Merged

Eslint config defaults #526

merged 3 commits into from
Sep 27, 2015

Conversation

taion
Copy link
Contributor

@taion taion commented Sep 25, 2015

Closes #522.

@justjake: I can see why you didn't want to do this. This was a lot more time-consuming than I had anticipated.

It's split up into 3 commits:

  1. Bring in the rules from eslint-config-defaults and plug them in.
  2. Account for the mismatches against what's currently in 0.0.9.
  3. Update for changes in latest versions of ESLint and eslint-plugin-react.

- react/jsx-quotes has been superseded by jsx-quotes
- eqeqeq now covers comparisons to null
module.exports = eslintrc;
module.exports = {
'extends': [
// Need to qualify these here for ESLint to resolve them properly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you not just reference them relatively? I think that issue was why I was constructing a rules object instead of just extending 'base' previously. Can eslint resolve these when running npm run lint without special linking magicks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm run lint didn't work when referencing them relatively. It interpreted them as the names of eslint-config-* packages for some reason.

It's possible that doing something like https://github.com/walmartlabs/eslint-config-defaults/blob/master/package.json#L34 involving making main not be index.js could work, but I didn't try it, because this approach seemed tolerable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe all extends in eslint have to be named starting with "eslint-config", or a scoped package with the name ending in "/eslint-config".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb I have no clue - the relative imports work here: https://github.com/airbnb/javascript/pull/526/files#diff-ca246d3b4ba146243647f872c0e7e8d1R3, but they didn't work on index.js. ¯_(ツ)_/¯

@justjake
Copy link
Collaborator

SO GOOD! I'm very excited for this; will make it easy to fix things like #496 in the next few PRs.

@hshoff you should look this over and make sure you're ok with this.
@baer how does this satisfy #366 for you? Is the structure similar enough? Or do you want index.js files in folders?

@hshoff
Copy link
Member

hshoff commented Sep 25, 2015

Lovely! Love open source. Thanks @taion! 🍻

justjake added a commit that referenced this pull request Sep 27, 2015
@justjake justjake merged commit 16f81fb into airbnb:master Sep 27, 2015
@taion taion deleted the eslint-config-defaults branch September 27, 2015 19:36
@baer
Copy link

baer commented Sep 28, 2015

This is super awesome guys! Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants