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

Disable ESLint rule conflicting with Prettier #55

Merged
merged 5 commits into from Oct 12, 2022
Merged

Disable ESLint rule conflicting with Prettier #55

merged 5 commits into from Oct 12, 2022

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Sep 15, 2022

Prettier does not have a way to configure a space before the parenthesis - they recommend disabling this rule

Apart from this rule, it seems all the rest of the preferences can be configured in Prettier (see #53)

.eslintrc.cjs Outdated
rules: {},
plugins: ['@typescript-eslint'],
rules: {
'space-before-function-paren': 'off'
Copy link
Owner

Choose a reason for hiding this comment

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

can we instead set prettier to have a space before function parenthesis? I'd honestly prefer that style since we are trying to stick as much as possible to StandardJS...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is not possible.

For better or worse (I believe zero- or low-config is actually better for the software development ecosystem in total - even did multiple tech talks about this) Prettier has chosen to not allow a lot of things to be configurable, and the best way to integrate is to disable these ESLint rules.

I remember also having the same objections at first, ceding absolute control over the smallest code style details in my projects, but after letting go, it was also a lifted burden.

Prettier is pretty widely used, and I would say that it helps contributions to allow users to use this.

One alternative that I just remembered that may be more appealing is to also extend eslint-config-prettier, which is their official maintained config disabling rules that conflict with Prettier.

Anyway, in the end I understand if you don't want to support Prettier - it's your decision.

Copy link
Owner

@lmammino lmammino Sep 16, 2022

Choose a reason for hiding this comment

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

I admit I haven't been a fan of prettier exactly for this kind of reason. I always felt it gets a bit in the way more than it helps. On top of that, I generally work with ESLint autoformatting which, in combination with StandardJS, is for me the lowest barrier to entry for consistent styling and auto-formatting. 1 single tool with convention-based config (rather than 2 tools with 2 configuration files)

Anyway, I see this as a community project, so if there's a strong interest in the community in using prettier, I might happy to change my mind (for this project), but otherwise, I'd prefer to skip prettier entirely and rely only on ESLint for style checking and formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there is definitely a good amount of community interest in Prettier, but can't make the decision about whether this outweighs your ownership / tool preferences - you should decide this.

As a side note: before Prettier, I also was all-in on ESLint autofixing and using it as my only tool. Moving to Prettier showed me how much faster and nicer a formatter can be, and now I've completely moved away from autofixing. In the future, I'm also looking forward to moving away from Prettier to a faster formatter in a lower-level language like Rome.

Copy link
Owner

Choose a reason for hiding this comment

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

I honestly find it baffling that prettier tries to be open to some configurations but then it tries to be strongly opinionated about certain things. Is it a formatter or is it a code style standard?

Rant aside, I had a quick try on this branch (after rebasing on main to get the config file we have there) and just formatting with prettier create a significant amount of changes in the code (which are not reflected in this PR, but that you can -partially- see in the screenshots below).

I'd be ok with prettier if it wouldn't require us to change the style (and therefore generate a lot of unnecessary code changes). The reason why I like to use StandardJS (although I know it's not a real standard) is to avoid this kind of conversation where we end up debating about which style is better and I don't think there's any way to win at that debate... pretty much like tabs vs spaces...

If you are really strongly opinionated about this, I can leave aside all my prettier grudges, but I'd like this PR to do the following:

  1. apply all the necessary formatting changes that prettier would require from now on
  2. have hooks and tests that check against the code style required by prettier

If we do all of that, is there still value in using eslint? Can we use prettier to also check the style and get rid of eslint entirely? If the answer is yes, can we do that as well as part of this PR?

PS (Just for context): when you added prettier in the other PR I assumed it would integrate nicely with eslint and be just another formatting option for people who have their environment already configured with prettier (rather than eslint). Now this seems like a more fundamental stylistic change...

Screenshot 2022-09-16 at 18 29 06
Screenshot 2022-09-16 at 18 29 38

Copy link
Contributor Author

@karlhorky karlhorky Sep 16, 2022

Choose a reason for hiding this comment

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

There are only few circumstances where I describe my feelings as strong, so I'm not sure if I'd characterize them so here. I think that using Prettier is probably a good, modern choice for a community project, but I totally understand if you want to keep it with how you've been doing all of your projects.

So I still want to leave the final decision up to you here, but here are some further data points:

  1. Prettier is a low-configuration (almost zero config) code formatting (style) tool. It's meant to replace the litany of options that tools like ESLint offer, while still having a few levers for more contentious, prominent style arguments like tabs vs spaces, semicolons vs none, etc. They have purposely tried to keep it low configuration to try to avoid the tragedy of the commons. Their options philosophy describes this further.
  2. I find that ESLint is still useful! For things non-stylistic like finding problems that would cause runtime errors in your program and unnecessary code (Prettier does nothing about this). At UpLeveled we maintain an ESLint config (and also plugin) that deals with this, focused on beginners. So ESLint for non-stylistic things that could cause runtime errors, confusing things, security problems or unnecessary code.
  3. I'm ok with doing the following in this PR too (and I understand why you want this):
    • Writing an npm script that will format all code with Prettier (npm run format in root)
    • Formatting all code with this script
    • Adding an npm script to check that all files have been formatted with Prettier (npm run test:format in root)
    • Adding a GitHub Actions step that runs this 2nd script as part of the linting process (runs as part of npm run test)
    • Adding the Prettier ESLint config to disable any conflicting rules

Let me know what you decide :)

Copy link
Owner

@lmammino lmammino Sep 17, 2022

Choose a reason for hiding this comment

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

Thanks, @karlhorky for indulging in this conversation with very valid points and really good examples. I also recently run a poll on Twitter and it seems that many people are more or less in line with the process you are describing here.

I am starting to get more what's the individual value of ESLint and Prettier and how they can be used together to get the best out of them. I am still a bit unsatisfied with that missing space before function arguments, but I guess I can live with that since this setup feels important to the community at large... 😅

This is just to say that I am OK with the approach you are suggesting at point 3. Do you want to do these changes as part of this PR?

Thanks again

Copy link
Contributor Author

@karlhorky karlhorky Sep 17, 2022

Choose a reason for hiding this comment

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

Great, glad to help :) Cool that you reached out to the community.

And sure, happy to do the changes. I'll take a look at them probably this weekend.

@karlhorky
Copy link
Contributor Author

Rebasing this on top of #56, so that we can avoid the duplication

@karlhorky karlhorky closed this Oct 12, 2022
@karlhorky karlhorky reopened this Oct 12, 2022
@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@4a6499b). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             main       #55   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         1           
  Lines           ?        35           
  Branches        ?         5           
========================================
  Hits            ?        35           
  Misses          ?         0           
  Partials        ?         0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@karlhorky
Copy link
Contributor Author

This should be good to go now @lmammino 🙌 I documented my progress up in my original comment above: #55 (comment)

@lmammino
Copy link
Owner

Thanks for the hard work on this @karlhorky

@lmammino lmammino merged commit 051fe05 into lmammino:main Oct 12, 2022
@karlhorky karlhorky deleted the disable-conflicting-eslint-rule branch October 12, 2022 18:06
@karlhorky
Copy link
Contributor Author

Glad to help, thanks for the merge! Now finally back to #43

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.

None yet

2 participants