Skip to content

Conversation

lexun
Copy link
Contributor

@lexun lexun commented May 16, 2017

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.

Copy link
Contributor

@randycoulman randycoulman left a 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).

@lexun
Copy link
Contributor Author

lexun commented May 17, 2017

Sounds good to me, although I would like to question the --single-quote and --no-semi options. For maximum community alignment, might it be worth adopting the defaults? I don't see much downside considering prettier will correct things for you regardless of what you type.

@randycoulman
Copy link
Contributor

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.

@randycoulman
Copy link
Contributor

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 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. 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 decided to also set the name property at the same time, so we no longer need to use an EJS template to insert the project name correctly.

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!

lexun and others added 4 commits June 12, 2017 15:04
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!
Copy link
Contributor Author

@lexun lexun left a comment

Choose a reason for hiding this comment

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

👍

@randycoulman randycoulman merged commit 2f7f7c9 into master Jun 16, 2017
@randycoulman randycoulman deleted the chores/prettier branch June 16, 2017 17:41
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.

2 participants