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

feat: add prettier support #231

Closed
wants to merge 1 commit into from
Closed

Conversation

theoludwig
Copy link
Member

@theoludwig theoludwig commented Dec 9, 2020

What is the purpose of this pull request? (put an "X" next to item)

  • Documentation update
  • Bug fix
  • New feature
  • Other, please explain:

What changes did you make? (Give an overview)

  • New CLI flag --format
  • New option format for engine.lintFiles
  • New options prettier and prettierConfig for new Linter()

See #230 for more information.

Which issue (if any) does this pull request address?

close #230

Is there anything you'd like reviewers to focus on?

BREAKING CHANGE: need to require prettier in options.js for new Linter()

@welcome
Copy link

welcome bot commented Dec 9, 2020

🙌 Thanks for opening this pull request! You're awesome.

@theoludwig theoludwig marked this pull request as draft December 9, 2020 23:45
@theoludwig theoludwig force-pushed the feature/prettier-support branch 2 times, most recently from 2587a5a to c367bd6 Compare December 10, 2020 13:38
@theoludwig theoludwig marked this pull request as ready for review December 10, 2020 13:38
@ungoldman
Copy link
Contributor

ungoldman commented Jan 8, 2021

I think standard is going to be eslint driven for the foreseeable future, perhaps this would be more appropriate as a fork than adding a lot of prettier-specific options under the hood. @standard/team thoughts?

@ungoldman
Copy link
Contributor

ungoldman commented Jan 8, 2021

I see there's a lot of previous discussion around this in issues linked in #230. I'm not familiar with this and don't have availability to dive in so I'm going to remove myself as a reviewer. Best of luck 🖖

@ungoldman ungoldman removed their request for review January 8, 2021 20:56
@theoludwig
Copy link
Member Author

theoludwig commented Jan 9, 2021

Thanks for sharing your thoughts! @ungoldman

In my opinion, using standard should be as easy as doing npx standard, it means no configuration and still "Catch style issues & programmer errors early".
Most JavaScript codebase doesn't only contain JavaScript code, it also contains json, yaml, markdown, css etc.
The problem with eslint is that it only runs on JavaScript code.

For example in standard, we have the rule of 2 spaces, why my CSS file would not have this opportunity to format with 2 spaces as my JavaScript code ?
There is a comparaison : Prettier vs Linters and it would be awesome if standard could provide a way to use Prettier for formatting and linters for catching bugs!

There is already a way to do it : prettier-standard but it doesn't support well TypeScript and I would rather that it would work out-of-the box with standard, ts-standard and others engine available in the standard organization and it would be more "official". 😄

Note: It is facultative, feel free to don't use prettier with standard, since the standard and standard --fix command work as it normally would.
But if you want to Aggressively format code for consistency, you can use standard --format so it runs prettier and then just after eslint.
As it is what this PR intend to do.

Concerning the way to support it with eslint, it is not a "problem" as long as you don't use code style rules in eslint (e.g: max-len, no-mixed-spaces-and-tabs, keyword-spacing, comma-style…) since it's the job of prettier.
For example with standard or ts-standard, it's only 4 lines of prettier configuration :

{
  "singleQuote": true,
  "jsxSingleQuote": true,
  "semi": false,
  "trailingComma": "none"
}

I already use it in VSCode with the prettier extension and ts-standard engine and yet I didn't have any conflict issue with eslint rules defined in eslint-config-standard-with-typescript and prettier, so I don't think it is a problem to make prettier get along with eslint rules.

@feross
Copy link
Member

feross commented Feb 23, 2021

@divlo This is something I've been interested in exploring for some time. I mentioned a bit about it here: standard/standard#1356

Thank you for taking the time to PR this. I haven't had a chance to dig in too deeply, but my first impression is that this PR seems straightforward enough and doesn't add too much code. I'll take a closer look at merging this when it's time to release the next major version of standard.

Copy link

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@epiqueras
Copy link

Looking forward to having this merged.

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Code looks good! 👍

@bnjmnrsh
Copy link

bnjmnrsh commented Apr 5, 2021

Hello! I see there are comments that this PR may be included in the next release, however, I haven't found a roadmap for this.

I've spent the last day or so exploring all the different combinations of configs and packages and haven't found a clear way forward for using semistandard and prettier (am currently hung up with the missing space before function parens conflict), and I'd rather not bring in more complexity with eslint (and friends), if I don't have to.

I'm happy to help with docs and other supporting materials if it will help.

Basically +1 for this! 🎉

@theoludwig
Copy link
Member Author

Hey! 👋 @bnjmnrsh
Actually as said in this comment : #231 (comment)
It is not hard to integrate, prettier with any of the standard engines available.

What this PR indents to do, is that when you will do semistandard --format, it will first format the code with prettier and then will fix issues with eslint as it does now, when you're doing semistandard --fix, it will fix all the errors made by prettier including missing space before function parens, the only things that can't be fixed, is thing that eslint can't fix, as it is the case with the current --fix flag.

While we're waiting for the next major release of standard, with this PR merged, you can already integrate semistandard and prettier together, first you need to create a new file called : .prettierc.json, then in this file you put this content :

{
  "singleQuote": true,
  "jsxSingleQuote": true,
  "semi": true,
  "trailingComma": "none"
}

You need to install prettier inside devDependencies of course, like so npm install --save-dev prettier, and then you can create a new format script in your package.json like so : "format": "prettier --write '**/*.{ts*,js*,yml}' && semistandard --fix" (for example).

It should work perfectly, if not, feel free to share with us your errors.

I'm happy to help with docs and other supporting materials if it will help.

Standard should be as easy as running one command, so I don't think, it is great to add too much documentation, as it should work without configuration out of the box, the only thing in the docs that need to be added, is the --format flag, but it is already mentioned with this PR in README.md.

Basically +1 for this! 🎉

Great! 😄

@bcomnes
Copy link
Member

bcomnes commented Apr 5, 2021

Does this still support a stdin/stdout api necessary for many editor formatting plugins?

@theoludwig
Copy link
Member Author

Does this still support a stdin/stdout api necessary for many editor formatting plugins?

The --fix flag stay unchanged, this PR only add --format flag, but the --format flag, directly update the files.
What "editor formatting plugins" are you talking about ? @bcomnes
We actually have an official vscode-standard plugin for VSCode, and we'll make sure that we can provide this new format option support, for this extension. We'll see how we could implement that, but first we need to make sure, that we update every official standard engines in the org to also use prettier, once this is done, we'll make sure to integrate it with every possible editor.

@bcomnes
Copy link
Member

bcomnes commented Apr 5, 2021

I maintain one of the sublime standard formatter plugins

https://github.com/bcomnes/sublime-standard-format

It makes use of the stdin api in order to format buffers prior to saving files. I'm not familiar with vscodes formatting api.

@bnjmnrsh
Copy link

bnjmnrsh commented Apr 5, 2021

@divlo thanks for the quick response 🚒 !
I realize I failed to mention one tiny detail: that I'm trying to implement this workflow within vscode, on file save (🙄 sorry!)

While your package.json script approach works well, I've been enjoying the highlighting that the standard vscode plugin provides, and had hoped that with the right rules, it would play nice with the Prettier plugin too. But as you likely know, I can't control (to the best of my knowledge) which plugin formats first the way we can with a package.json script. That's why I'm seeing the semistandard(space-before-function-paren) rule be applied, and then removed on each save ↻↻ 🌊

To get around this, I've seen folks use an on-save plugin, and there is the standard-prettier-vscode plugin which I didn't get results from -- at nine stars, its a bit untested.

And failing the above, you're in the realm of trying to couple something like eslint-config-semistandard with prettier-eslint, which has its own vscode plugin. If there's a way through that solution, I've not found it or found evidence from anyone else who has. At any rate, by this point, you've thrown a ridiculous amount of tooling at something that you seemed soo tantalizingly close to a few hours ago! ... 🌈 🍯

The missing piece seems to be this PR ✨

@theoludwig
Copy link
Member Author

I maintain one of the sublime standard formatter plugins

https://github.com/bcomnes/sublime-standard-format

It makes use of the stdin api in order to format buffers prior to saving files. I'm not familiar with vscodes formatting api.

Right I don't know either, feel free to open a PR against this branch so I can integrate it with this PR to implement stdin output with the prettier support.

Also I guess the missing piece for this PR is some automated tests! 😄

@theoludwig
Copy link
Member Author

Indeed! @bnjmnrsh
This PR, will effectively simplify a lot the integration of standard with prettier, you would be able to have prettier + semistandard really easily without too much configuration, it would work out of the box, but as it is now, I guess, we need to wait the new major release of standard.

@bcomnes
Copy link
Member

bcomnes commented Apr 5, 2021

In addition to the lack of stdio api, my main hangup here is what advantage does this provide over installing standard and prettier, and configuring a lifecycle script that runs prettier && standard --fix? Just a small shortcut around that step?

It seems like the primary location formatter tools are used are in the editor, and the snag people get hung up on is using prettier with standard (e.g. producing a format with prettier that standard doesn't like, and trying to reconcile that only to have prettier break it again because people want format on save). They install prettier and format on save and then get annoyed when standard doesn't like the result. Part of this is inflexibility is due to editor plugins only working one way, but more fundamentally its a competing ruleset problem.

We've tried to maintain separate formatting rules from the eslint rules ages ago, and it didn't work well. One problem we ran into was how do we guarantee that prettier (or whatever separate formatter rule we are using) doesn't format into something that standard --fix can't fix, or worse fixes in an unintended way and introduce a bug.

It seems like two better options exist:

  • What about just incorporating more of the prettier eslint rules into standard, and continue to rely on standard --fix (e.g. eslint --fix). This way you eliminate the competing rule problem and get a more aggressive format. Formatting with a broken result then fixing sort of works, but it's not a correct solution to the problem. Aggressive formatting preferences is an appropriate linting rule, and is addressable with eslint --fix when its tested for.
  • If it has to be actual prettier + actual standard consumed in one official command that runs them on top of each other, prettier should be an optional peer dependency that is guarded for on --format flag. But even this feels like you are bifurcating standard into two code styles (fixed and formatted).

@theoludwig
Copy link
Member Author

what advantage does this provide over installing standard and prettier, and configuring a lifecycle script that runs prettier && standard --fix? Just a small shortcut around that step?

As said standard should be easy to use and provide useful/pretty code styles and rules and that goal without the need of configuration or complex usage of many tools, standard does all that stuff for you automatically.
As a user, you don't need to do much things, you just need to do npx standard --format and you've got a "pretty" codebase, I guess that the idea behind standard : easy to use, automatically format code and catch style issues & programmer errors early.

It seems like the primary location formatter tools are used are in the editor

Not only, but yes, you're right, it is mostly used in the editor. Actually, personnaly, I also use a lot, the command line.

We've tried to maintain separate formatting rules from the eslint rules ages ago, and it didn't work well. One problem we ran into was how do we guarantee that prettier (or whatever separate formatter rule we are using) doesn't format into something that standard --fix can't fix, or worse fixes in an unintended way and introduce a bug.

I completely understand that, but in fact it is not what prettier and eslint should do together.
Must read: https://prettier.io/docs/en/integrating-with-linters.html

TL;DR: Linters usually contain not only code quality rules, but also stylistic rules. Most stylistic rules are unnecessary when using Prettier, but worse – they might conflict with Prettier! Use Prettier for code formatting concerns, and linters for code-quality concerns, as outlined in Prettier vs. Linters.

What about just incorporating more of the prettier eslint rules into standard, and continue to rely on standard --fix

It is not enough, because eslint is only running inside JavaScript files, not in HTML, CSS, JSON, Markdown files etc, and as said in previous comments, why would my JavaScript files would have 2 spaces indentation but not my CSS for example, that's what prettier could actually solve, and we could not solve it with this first option.

If it has to be actual prettier + actual standard consumed in one official command that runs them on top of each other, prettier should be an optional peer dependency that is guarded for on --format flag.

Currently, what this PR will provide to standard engines, is to provide a prettier instance as they do with eslint, and run prettier && eslint --fix, so currently it include prettier in the dependencies as it is also the case with eslint.

And for the actual users, they don't need to run standard --format, if they don't feel to, feel free to only use standard --fix, if that's what they want.

Thanks for sharing your opinions! @bnjmnrsh
What do you think? 😄

@bnjmnrsh
Copy link

bnjmnrsh commented Apr 5, 2021

@bcomnes, I can see where you are coming from with these two points. Mind you that a lot of this has been discussed quite extensively (#1356, #811, #996, among others), and I can see that you have been a part of many of them.

So, I hope you'll all pardon some observations from a newcomer. In researching this over the last few days, I was dismayed to realize that there has been an impasse on this integration, from the looks of things, dating back to the early paleolithic era of 2017. From what I can gather, it appears that major sticking points are a subset of rules, that for whatever reason, the Preitter team doesn't wish to make an exception for (I mean, where would it end?!). And over in the Standard camp, it would appear they also don't want to bring their rule set into alignment, either fully or behind a feature flag.

Meanwhile, I'd hate to see the sum of human-hours that have gone into the literally dozens of plugins, config attempts, feature requests, and midnight-searching-of-the-soul of those in userland like myself, looking for what surely-must-be-a-feature-that-I've-just-not-done-the-right-google-search--for. I mean, 'all' I want is to type my code, have it lint with the little red wiggles when I mess up, and for it to be pretty on save?! 😅

On @bcomnes specific points:

What about just incorporating more of the prettier eslint rules into standard and continue to rely on standard --fix (e.g., eslint --fix). This way, you eliminate the competing rule problem and get a more aggressive format...

Adding to @divlo's comments about the pervasive use of Prettier across a project, I can see why the idea of incorporating more of Prettier's rules into Standard is compelling, but if it were easy, it surely would likely have been done by now. Also, it means trying to track the output against Prettier as that protect grows and develops, which seems to be an ambitious commitment. IMHO formatting is Prettier's Jam, so leverage that, why try to be all things? This way lies madness.

I also see what you're saying with your second point.

... prettier should be an optional peer dependency that is guarded for on --format flag.

I absolutely agree with this. If you don't install prettier as a dependency, I personally would expect it would throw a warning and fail—no prettier, no --format soup for you.

However...

But even this feels like you are bifurcating standard into two code styles (fixed and formatted).

Isn't this what is happening already, very poorly?
Every solution I've looked into either applies Prettier after Standard or tries to mock Prettier with eslint and then pushes it through Standard. The former 'alters' Standard, as you say. The latter adds a whole mound of complexity that makes my projects that much harder to set up and maintain. Furthermore, there isn't any clear route using semistandard, as most projects only have examples for use with standard.

Barring some compromise on the part of one of the two camps - what we have here, ladies and gentlemen, is known as a good old fashion _ "impasse"_, and a compromise is surely called for (I think that's the needle this PR tries to thread).

I know I've glossed over a lot of complexity, but hopefully not mischaracterized anything, or anyone's position.

In the meantime, if the past is prologue, as there isn't any particular indication as to when this PR or underlying issue might be addressed, I propose some supplementary documentation that outlines the successful approaches to achieving these same results until this PR is merged. Again, I'd be happy to contribute to such an effort, once I'd figured it out! 😀

@KidkArolis
Copy link

I'm all for making standard and prettier work well together out of the box.

And while the discussions continue going on, just wanted to share the solution I've been happily using for the last 2+ years. (which I really hope you don't mind me sharing here @feross (and co), fan of all the work you do 🙏).

I run prettier on save to format the code, and run healthier to lint the code. Healthier checks all standard's rules that don't conflict with prettier. And it comes with vscode, sublime and vim plugins. Been working well for me, maybe it can work for you too, at least until some more native solution (as discussed in this PR) comes along.

In fact, this has been working so well.. that I would even suggest exploring an alternative solution to combining prettier and standard. Leave formatting entirely to prettier (including it's editor plugins, config files and all that jazz) and add a flag to standard that removes styling rules. This way the answer to anyone asking how to use both is to use prettier && standard --no-style or smth like that. This way healthier would pretty much no longer be needed as all it does (or tries to do) is run standard engine with styling related skipped (thanks to prettier/standard eslint config and other bits).

@bnjmnrsh
Copy link

With great respect to the work that @divlo has already done on this PR, @KidkArolis suggestion has a lot of merits to consider, while minimizing possible downstream effects.

It retains the benefit of this PR by not changing the base output of standard (without the flag) but also sidesteps the issue of tight coupling with the Prettier project, i.e., it doesn't cause the breaking change of requiring prettier in options.js.

A decoupled approach also means that should someone prefer another formatter, then there would be little if anything standard would have to do to support it. Downstream, I imagine that this approach may also require less effort to accommodate across the ecosystem, editor plugins, ts-standard, semistandard, etc.

@voxpelli
Copy link
Member

@divlo This needs a rebase now

@theoludwig theoludwig modified the milestones: 15.0.0, 16.0.0 Dec 7, 2021
@theoludwig theoludwig removed this from the 16.0.0 milestone Apr 20, 2022
@theoludwig
Copy link
Member Author

This PR has no recent activity, and probably won't be merged for a long time and as I don't want/have time to do the job for this PR, I decided to close it.

Feel free to open another PR yourself based on this one if you want this to be implemented.

@theoludwig theoludwig closed this Apr 20, 2022
@theoludwig theoludwig deleted the feature/prettier-support branch April 20, 2022 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Prettier support