-
Notifications
You must be signed in to change notification settings - Fork 2
Format code with prettier #62
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of moving to Prettier, but we're about to release 1.0, so I'd like to hold off on this for now.
To move forward, I'd suggest the following:
- Use the
--single-quote
and--no-semi
options and reformat the code again - Add a pre-commit hook to automatically format staged files as described in the prettier docs
- Update eslint-config-zeal for the use of prettier. We need to do this in a way that works for projects that are using prettier and those that aren't (yet).
Sounds good to me, although I would like to question the |
I think the community will eventually align on prettier as a tool, but I don't think they'll align on a common set of options. For example, some teams will turn on trailing commas and others won't. So I think it's reasonable to pick the settings we want. My personal preference is to turn those two options on because I find they reduce the visual noise in the file when I'm trying to read the code. |
I've addressed the issues from my review on behalf of @lexun. I added eslint-config-prettier and added it to our generated eslint configuration file. I also added a git pre-commit hook to check that files have been formatted first. With this change, all staged *.js files checked for prettier-compatible formatting and the commit will fail if any are unformatted. Ideally, we'd have prettier reformat the files for us, but there are issues when there are files with only partially-staged changes. See lint-staged/lint-staged#62 for details. In the generator project itself, the git pre-commit hook will be installed in the parent project whenever To solve this, I changed the way we install the I decided to also set the So now, the |
This avoids any fights between prettier and our standard eslint configuration.
Accidentally did it in the parent project on the last commit.
With this change, all staged *.js files checked for prettier-compatible formatting and the commit will fail if any are unformatted. Ideally, we'd have prettier reformat the files for us, but there are issues if there are files with only partially-staged changes. See lint-staged/lint-staged#62 for details. In the generator project itself, the git pre-commit hook will be installed in the parent project whenever `yarn` is run in the templates directory (which happens a lot during development). However, lint-staged doesn't work in this context unless we add a `gitDir` setting to the lint-staged configuration. However, we don't want that setting in the generated project. To solve this, I changed the way we install the `package.json` file. We read the template file, modify it in memory to remove the `gitDir` setting, and write it out to the target directory. I also set the `name` property at the same time, so we no longer need to use an EJS template. So now, the `package.json` file is always valid and we don't have to edit it in order to run `yarn` commands during development. This is a much nicer workflow!
ceaf695
to
efc8296
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Take two...
#23
I'm happy with where prettier is at this point, and would like to adopt it with the standard configuration to be in alignment with the rest of the community. Thoughts?
Obviously there would have to be some lint configuration updates before we could merge this.