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

Update ESLint Config #407

Closed
kwelch opened this issue Apr 5, 2017 · 23 comments
Closed

Update ESLint Config #407

kwelch opened this issue Apr 5, 2017 · 23 comments

Comments

@kwelch
Copy link
Collaborator

kwelch commented Apr 5, 2017

After the changes made in #405, there is a lot of eslint config now in the package.json.

In effort to clean this up, there was a suggestion to move Slingshot to a baseline preset.

This issue is to capture that ask and open the discussion to others about what the eslint config preset should be set to.

Options

  • ESLint Recommended Baseline
    • Do we include other recommended baselines (ie. react, import, etc.)
  • ESList shared preset
  • Package Slingshot Config
    • Is the current config at a place that is right sized and can be shared as a packaged config
@coryhouse
Copy link
Owner

Thanks @kwelch! I vote for ESLint recommended. If you prefer something else, enter a comment so people can upvote via a 👍

@kwelch
Copy link
Collaborator Author

kwelch commented Apr 6, 2017

I would not mind seeing us incorporate prettier (or prettier-eslint) to have a simple consistent format and use baseline recommended for best practice / bug catching type linting.

@coryhouse
Copy link
Owner

coryhouse commented Apr 6, 2017

I like Prettier, but also recognize it's a polarizing idea - As an anecdote, in our office, a significant portion of people find the idea of having their code changed automatically in any way unsavory. Many prefer to "learn from their mistakes" by manually fixing what ESLint reports. I can appreciate that. And there are certainly things Prettier does that I don't like - such as splitting long imports into multiple lines. So I feel ESLint recommended is the most populous friendly approach at the moment.

@kwelch
Copy link
Collaborator Author

kwelch commented Apr 7, 2017

I understand that. It also adds a bit more weight than I think we would stand to institute and support in a boilerplate. It is a better fit for library.

@kwelch
Copy link
Collaborator Author

kwelch commented Apr 11, 2017

I think we are solid on the approach. Adding a help wanted and good first PR label.
😄

@qborreda
Copy link

@coryhouse - you could have both, ESlint would still throw errors and warnings at you via your editor, while prettier can make sure the output of the file is exactly the same no matter who has written it.

@coryhouse
Copy link
Owner

@qborreda True, though it seems the recommended approach is to disable any formatting related warnings from ESLint to avoid the duplication. It would seem noisy to warn someone about something that will be automatically fixed on save/commit. I think that's the sales pitch for prettier - it makes coding more luxurious because you literally stop thinking about formatting altogether.

@mikedevita
Copy link
Contributor

mikedevita commented Jun 28, 2017

I am not a maintainer but I do use this boilerplate quite a bit at my 9-5 and on my side work.

I prefer standard and usually end up bundling it in along with stylelint-config-standard. I ended up forking the repo and making those changes along with any other specific changes.

@nickytonline
Copy link
Collaborator

@mikedevita, I haven't done a project with either yet, but just curious why you prefer standard over prettier. I know prettier has a setting to kill semi-colons if you don't want them.

@mikedevita
Copy link
Contributor

mikedevita commented Jun 28, 2017 via email

@kwelch
Copy link
Collaborator Author

kwelch commented Jun 28, 2017

I think for this project standard is too opinionated. I would recommend we move forward with a slim lint config that covers potential errors/bugs.

@jacqueswho
Copy link

so regarding your rules in this template. you based it off airbnb . But made some modifications?

@romarioraffington
Copy link
Contributor

This seems simple enough for a first PR.

So the consensus was to use ESLint recommended?

Seems like this was already done:

"extends": [
      "eslint:recommended",
      "plugin:import/errors",
      "plugin:import/warnings"
    ],

cc @coryhouse

@kwelch
Copy link
Collaborator Author

kwelch commented Aug 16, 2017

The part we want to simplify is the rules section of the package.json

Currently the eslint section of the package.json is just far too long and for this type of project too opinionated in my opinion. Anything that is style based can be remove in favor or prettier. Main focus should be on the mitigating potential errors.

@romarioraffington
Copy link
Contributor

Okay so:

  1. Remove rules from package.json
  2. Use prettier or prettier-eslint to format the code based on the rules we removed in step 1 ?

@kwelch
Copy link
Collaborator Author

kwelch commented Aug 16, 2017

Not sure.

@coryhouse & @nickytonline,

How do you feel about the eslint rules? Should adopt or create a single preset that has all options needed? Or should we just remove rules and use the presets we currently have?

Please vote:
👍 - Create eslint-config-slingshot
🎉 - Remove all rules
❤️ - Use Preset (reply with recommended config)

@nickytonline
Copy link
Collaborator

nickytonline commented Aug 17, 2017

@kwelch, since this is a boilerplate, I'd either remove all rules or go with a standard preset.

@kwelch
Copy link
Collaborator Author

kwelch commented Aug 17, 2017

Agreed. Any recommendations? I don't think the presets we have are very helpful safety for a starter.

@coryhouse
Copy link
Owner

I agree that the loose settings we have now aren't the ideal - they're really my unique preferences. I'd suggest just using a standard preset.

@mattiaerre
Copy link
Contributor

@coryhouse @kwelch so we would like to have a preset but Airbnb is not a good candidate? what about eslint-config-standard and eslint-config-standard-react. I would also move all the rules to a .eslintrc file. Happy to attempt a PR if you like.

@coryhouse
Copy link
Owner

It looks like remove all rules received the most votes above so I'd suggest a PR that:

  1. Removes our custom rules.
  2. Just calls ESLint recommended.

Thoughts @kwelch @nickytonline ?

@nickytonline
Copy link
Collaborator

Sounds good to me.

@kwelch
Copy link
Collaborator Author

kwelch commented Jan 3, 2018

Yes, I love that idea.

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

No branches or pull requests

8 participants