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

Add .eslintrc.json to support use of Prettier code formatter #19296

Merged
merged 1 commit into from May 9, 2019

Conversation

Projects
None yet
3 participants
@nathansobo
Copy link
Contributor

commented May 9, 2019

On other projects, I've used the Prettier code formatter via the official Atom package. It saves a lot of time hassling with code formatting, and not having it enabled for Atom is causing me to waste a lot of time fixing linting errors.

On Atom, we're using JS Standard Style, which conflicts with the Prettier's default formatting. This is okay, because the Prettier package can run eslint --fix automatically after performing its own formatting, but it requires a bit of configuration. To support it, I've added an .eslintrc.json file to the root of the module. This needs to pull in the shared ES Lint configurations associated with Standard Style, but since we avoid installing dev dependencies in our root package.json, I've added those linting configurations to script/package.json and am including them with an explicit path.

With this change, you can now get nice auto-formatting when you install Prettier Atom and enable ES Lint support in the package settings.

Screen Shot 2019-05-09 at 10 40 24 AM

We have a lot of files that aren't formatted with prettier, so this might generate a bit of churn initially, but I think the productivity savings will be worth it in the long run.

One particular issue is that Atom's source files don't have a consistent style with respect to spaces inside of curly brackets. Prettier defaults to having spaces ({ foo: "bar" }, but standard doesn't seem to care, and we have examples of both styles in this repo. It's possible to override the default and remove spaces, but it's more typical style to include these spaces, so I'm going to leave the current default in place.

cc @atom/maintainers

@Arcanemagus

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

I'd recommend adding a compatible version of eslint as a dependency as well so it is tracked with the plugins that use it.

@nathansobo

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@Arcanemagus the plugin I'm using bundles its own version of eslint, and I'm reluctant to mess with the linting infrastructure right now. I'm going to merge this. If there's something I'm not fully understanding however about the benefits then I am open to a follow-on PR.

@nathansobo nathansobo merged commit e15f9fa into master May 9, 2019

2 checks passed

Atom Pull Requests #20190509.5 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nathansobo nathansobo deleted the ns/eslintrc branch May 9, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

That's cool! I'm going to try it!

As a followup, it would be cool to add first-class support for prettier (so it can be executed across all codebase by running script/lint --fix or something similar) and introduce the eslint rule to fail if any file is not prettified.

This way we avoid files going back and forth in codestyle depending on whether the committer is using prettier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.