Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

feat($prettier-eslint): Integrate prettier-eslint #61

Merged
merged 2 commits into from
Mar 5, 2017

Conversation

robwise
Copy link
Collaborator

@robwise robwise commented Mar 4, 2017

As discussed in:

This commit completely refactors the repo. We now allow using
prettier-eslint as an option so that you do not need to install
a separate plugin (prettier-eslint-atom).

Under the hood, almost all of the code has been refactored and
covered by Jest integration tests. We also implement Babel and
many of the devops/build configurations that Kent Dodds authored
in the prettier-eslint-atom repo.

Resolves #57

@robwise
Copy link
Collaborator Author

robwise commented Mar 4, 2017

I could use help from the community with some manual acceptance testing because this is a big change under the hood. I have unit tests, but they rely heavily on mocking, so we're sort of not covered at the integration level.

cc/ @kentcdodds

As discussed in:
- #57
- kentcdodds/prettier-eslint-atom#28
- prettier/prettier#758

This commit completely refactors the repo. We now allow using
prettier-eslint as an option so that you do not need to install
a separate plugin (prettier-eslint-atom).

Under the hood, almost all of the code has been refactored and
covered by Jest integration tests. We also implement Babel and
many of the devops/build configurations that Kent Dodds authored
in the prettier-eslint-atom repo.

Resolves #57
@kentcdodds
Copy link
Member

Awesome @robwise!

Looks like there are some duplicate options:

screen shot 2017-03-04 at 1 25 09 pm

@kentcdodds
Copy link
Member

Other than that, the experience was pretty seamless! Tried both with prettier-eslint enabled and disabled and it worked as expected.

Question... Do you forward along the prettier options to prettier-eslint? Probably should do that if not :)

Thanks so much! I can't wait for this to be a thing :)

@robwise
Copy link
Collaborator Author

robwise commented Mar 4, 2017

Looks like there are some duplicate options:

@kentcdodds I've run into this duplicate options problem on my end as well, but I'm not sure what's causing it. I've got the Single Quote duplicate that you've got in your screenshot, but interestingly, I do not have the Bracket Spacing, Format On Save, and Print Width duplication. I even searched the entire repo for Single Quote and it only exists once. I will investigate further.

Question... Do you forward along the prettier options to prettier-eslint? Probably should do that if not :)

Currently, no, I don't format the prettier options if prettier-eslint is enabled. If you were to forward the options, wouldn't that have no effect since the ESlint rules would just revert those changes back (excepting the parser option, I suppose)? This is the same reasoning as what I said in kentcdodds/prettier-eslint-atom#12 (comment).

Also, I'm not sure how I would handle that in the settings. Would we add an overrides eslint checkbox for every single option? I feel like that would get really messy.

@kentcdodds
Copy link
Member

wouldn't that have no effect since the ESlint rules would just revert those changes back (excepting the parser option, I suppose)?

Sort of, there are some things that prettier-eslint can't infer properly (or at all yet) so it'd be nice to be able to specify them.

Also, I'm not sure how I would handle that in the settings. Would we add an overrides eslint checkbox for every single option? I feel like that would get really messy.

Good question. I think that people would naturally assume that their options they set will override the eslint inference... Maybe?

@kentcdodds
Copy link
Member

If we don't forward prettier options, then we need to make very clear that the prettier options aren't applied when using the eslint integration. Can we hide options based on the selection?

@robwise
Copy link
Collaborator Author

robwise commented Mar 4, 2017

Can we hide options based on the selection?

I was thinking the same thing, but AFAICT Atom doesn't allow doing that. For right now, I had to resort to this:

What do you think? Not obvious enough?

On the duplicate options thing, I tried moving the config schema to its own file and then importing it (which I like better anyway since it keeps the package.json from getting so out of control), but I still can see the duplicate option despite it definitely not existing in the repo. I'm sort of crossing my fingers that it will fix itself if I publish, but that's just a total guess. I've been trying to Google this issue, but so far I haven't found anything.

Previously, this package's settings were defined in the `package.json`. We are now moving that
config schema to its own JSON file, importing it, and then re-exporting it in the package's module
exports. This will help keep the package.json more manageable in the long-run.
@robwise robwise merged commit 536b449 into master Mar 5, 2017
@robwise robwise deleted the robwise/prettier-eslint branch March 5, 2017 15:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants