Use prettier to manage code style and formatting #1476
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cruikshanks
changed the title
Cruikshanks
changed the title
Cruikshanks
changed the title
Cruikshanks
force-pushed
the
pretty-branch
branch
from
8ac2cb7
to
ec76f04
Compare
Cruikshanks
added
housekeeping
|
Quick comment to flag up that once this PR is merged, we'll want to introduce a Also just to note that I don't think we documented how to make use of that file -- it's automatic in GitHub but required manual config locally, or it did when we introduced it at least. I think we all configured it ourselves locally at the time but any new (or returning!) devs won't have this configured so we'll want to update our install process to run |
When we ran this through chatgpt it suggested we should switch this off. Googling for the rule in connection with prettier suggests there is an incompatibility between the two, hence best to switch it off. But having double checked, the incompatibility is specific to [eslint-config-prettier](https://github.com/prettier/eslint-config-prettier?tab=readme-ov-file#arrow-body-style-and-prefer-arrow-callback) and its fix functionality. Lots of blog posts that cover setting up prettier + eslint include adding eslint-config-prettier as an additional package to deal with any issues between the two. However, when you dig in it is because they are also telling you to add eslint with its recommended rules. eslint-config-prettier then steps in and switches off those that prettier will handle instead. But if you are specifying the rules as we are, rather than using the recommended ones, there is no need to add eslint-config-prettier. In fact prettier, [recommends avoiding it if you can](https://prettier.io/docs/en/integrating-with-linters.html). > When searching for both Prettier and your linter on the Internet you'll probably find more related projects. These are generally not recommended[.] eslint-config-itself states this rule is safe to use if > You don't use eslint-plugin-prettier. In other words, you run `eslint --fix` and `prettier --write` as separate steps. So, we add it back in to ensure our existing convention is followed.
Prettier will format it if present but won't add it as it is seen as a code rule, not a style.
This is admittedly where we could be seen to "cross the streams". prettier doesn't format JSDoc comments. There is a plugin (https://www.npmjs.com/package/prettier-plugin-jsdoc) though its not official. When we checked it we found its options were limited compared to eslint-plugin-jsdoc. Critically, there is no option to tell it to ignore @Private or _myPrivateMethod() methods. It also doesn't have the same level of support as eslint-plugin-jsdoc. So, though it could be argued it is more concerned with 'style', we'll continue to use eslint-plugin-jsdoc to lint our JSDoc comments.
This will confirm that prettier in the CI fails the build because of the invalid formatting.
By not referencing StandardJS as an extension any more there are a number of StandardJS rules we'd be missing if we left the config as is. The problem is StandardJS concerns itself with a number of the same rules as prettier. So, we first copied all the core ESlint rules from https://github.com/standard/eslint-config-standard into our config (the ones that don't depend on an eslint plugin). We then ran https://github.com/prettier/eslint-config-prettier against them to highlight any issues. There were quite a number. We removed all that were flagged, though we are going to go back and revisit this output in the next few changes. ``` The following rules are enabled with config that might conflict with Prettier. See: https://github.com/prettier/eslint-config-prettier#special-rules - curly - no-tabs The following rules are enabled but cannot be automatically checked. See: https://github.com/prettier/eslint-config-prettier#special-rules - no-mixed-operators - no-unexpected-multiline - quotes ``` We'll also look at the rules dependent on plugins that eslint-config-standard would add.
https://eslint.org/docs/latest/rules/curly > JavaScript allows the omission of curly braces when a block contains only one statement. However, it is considered by many to be best practice to never omit curly braces around blocks, even when they are optional, because it can lead to bugs and reduces code clarity. StandardJS rules were enforcing curly only if the block, for example `if`, was multiline (`curly: ['error', 'multi-line'],`). With that configuration this would be valid ```javascript if (cart.items && cart.items[0] && cart.items[0].quantity === 0) updateCart(cart); ``` But if Prettier considered the line to long it would amend it to this. ```javascript if (cart.items && cart.items[0] && cart.items[0].quantity === 0) updateCart(cart); ``` That would then cause `curly` to fail. However, it is no longer an issue to use the rule if you avoid `'multi-line'` or `'multi-or-nest'` as the option and stick with `'all'`. That works for us, as it is already an unwritten convention that we always wrap our blocks, even one-line `if`, in braces. So, we add the rule back in but use the `'all'` option. Running **eslint-config-prettier** doesn't flag this as an issue any more.
- https://eslint.org/docs/latest/rules/no-tabs - https://eslint.style/rules/default/no-tabs Good old JavaScript. It made dealing with this rule fun! So, the core rule `'no-tabs'` conflicts with prettier, because it allows you to indent using tabs or spaces. Using `'no-tabs': 'error'` (which is how eslint-config-standardjs applies it) would find fault with that if prettier added them in. Because of our .editorconfig file Prettier knows not to use tab characters. But to ensure there is no conflict eslint-config-prettier recommends using `"no-tabs": ["error", {"allowIndentationTabs": true}]`. The fly in the ointment, is that `'no-tabs'` as a rule has been deprecated from ESLint from version 8. So, we could add it back in as eslint-config-prettier recommends. But then we are purposefully adding back in something that will eventually disappear. The reason it is deprecated is that all stylistic ESlint rules were moved to https://eslint.style/ when ESLint v9 (which we are on) was released. We still don't want any tabs in our code so we want this rule. But we believe the more forward looking solution is to reference the plugin ESlint expects you to use, even if that does mean we have to add the stylistic plugin back in!
- https://eslint.style/rules/default/quotes - https://eslint.org/docs/latest/rules/quotes Very similar to `'no-tabs'`, this is a core rule that eslint-config-standard adds in that eslint-config-prettier flags as an issue. But eslint-config-standard is also referencing the deprecated version of the rule. It has been moved to https://eslint.style/ from v9. For reference, what eslint-config-standard is trying to do is ensure you don't inadvertently declare a string using backticks that doesn't need them, for example `hello, world`. Quotes also covers how you escape quotes, for example, if like us you use single quotes you want to still allow `"Hello world's"`. eslint-config-prettier's concern is that your prettier `"singleQuote": true` is consistent with your `'quotes'` rule. Fortunately, ours is so we can add this back in, but we'll reference the non-deprecated version.
The last two core rules left to contend with were - https://eslint.org/docs/latest/rules/no-mixed-operators - https://eslint.org/docs/latest/rules/no-unexpected-multiline eslint-config-prettier explains why these my conflict with what prettier does - https://github.com/prettier/eslint-config-prettier?tab=readme-ov-file#no-mixed-operators-deprecated - https://github.com/prettier/eslint-config-prettier?tab=readme-ov-file#no-unexpected-multiline Essentially, they are both purely stylistic and there is no configuration of them that would prevent prettier from being an issue. So, we have opted to stick with the purpose of this PR; we let Prettier decide!
Now we are now dealing with rules eslint-config-standard uses to replicate StandardJS, but which aren't found in the core ESlint ruleset. The first of these is eslint-config-import which "support[s] linting of ES2015+ (ES6+) import/export syntax, and prevent issues with misspelling of file paths and import names".
With our config in its current guise, linting is now a separate 'thing' to formatting. That is of course unless the code you've added is incorrectly formatted. In which case we need to lint it?? To check? Don't we?! Some of confusion comes from Prettier's own docs. On the front page you see. > If you use ESLint [we do for code-quality rules], install eslint-config-prettier to make ESLint and Prettier play nice with each other. It turns off all ESLint rules that are unnecessary or might conflict with Prettier. Along with that is a link to "Integrating with Linters". In there you'll find > When searching for both Prettier and your linter on the Internet you'll probably find more related projects. These are generally not recommended[.] When we started this journey, we veered towards the second instruction. We focused on getting prettier setup and working as the primary tool. We then added ESLint in separately and treated them separately. But in use (when working on this branch) there different ethos and approach started to jar. Prettier (IMHO) is assuming you are just going to have your chosen IDE auto-format stuff on save. It views red squiggles as noise and annoyance, so developers should type away and let the plugin clean up afterwards. ESLint on the overhand believes you want to know there is an issue, so leans into highlighting issues in the IDE assuming you'll fix as you code. Auto-fix is available, but as not all issues are covered it needs to let you know as you code. Step forward [eslint-plugin-prettier](https://github.com/prettier/eslint-plugin-prettier)! > Runs Prettier as an ESLint rule and reports differences as individual ESLint issues Including it in our setup means prettier issues are now highlighted in the IDE along with all other ESLint code-quality issues. This means those who align with the ESLint ethos (highlight and fix as you code) can follow that workflow. But as we still depend on keeping the prettier config (eslint-plugin-prettier will automatically use it) along with having to install the prettier dependency, those who prefer to just have the IDE clean up on save are free to do so. We can move back to a single lint task both locally and CI, whilst devs have the flexibility in how issues are resolved (manually, ESLint auto-fix, or prettier + IDE format on save).
I happened to hit upon one of the previous issues we'd come across when considering how we could get ESLint and StandardJS to work together. [Support for Eslint v9 Flat Config format](standard/eslint-config-standard#411) was one of the blockers that meant to have both ESLint and StandardJS play ball, we were stuck on ESLint v8. I spotted that there had been some [recent activity](standard/eslint-config-standard#411 (comment)), all of which referenced an alternative called 'neostandard'. And oh my! All my issues/dreams were answered - Built for ESlint to avoid the need for separate IDE tooling - Built for the latest ESLint (v9) so flat-file config is supported - Just like we did, any style rules have been updated to use @stylistic/eslint-plugin - A desire to work with current practices. So, banning or requiring ; is an option, along with disabling style rules for those opting to use prettier For context, maintenance on StandardJS and related packages like eslint-config-standard has been stalled for some time. neostandard references the issue as being a [block in governance and direction of travel](standard/standard#1948). I've not been through every message, but it appears the maintainers are split between those who remained committed to StandardJS's 'one-tool one way' approach and those looking to move to where most folks are: ESLint. Even our own @johnwatson484 [has gotten involved!](standard/standard#1948 (comment)) The thread suggests that those behind StandardJS are open to reconciling the neostandard fork with StandardJS. But that comment was made some time ago. My bet is neostandard is here to stay as the successor to StandardJS. So, this change strips out all my hand-cranked implementations of the StandardJS rules, including those it was implementing from other plugins and replaces them with neostandard. Lovely!
Or I think so. One thing I'm concious we're missing is our extra rule that enforces `.js` when requiring files. We were getting this from [eslint-plugin-import](https://github.com/import-js/eslint-plugin-import) via StandardJS using the `'import/extensions': ['error', 'always']` rule in our old config. neostandard does not currently include eslint-plugin-import or the rules StandardJS was using from it. This is because it didn't support ESLint's flat-file config [until recently](un-ts/eslint-plugin-import-x#122). neostandard has a [PR open](neostandard/neostandard#197) that re-enables the 'import' rules. At which point we can piggy back again and add our extra 'import/extensions'. Activity looks good so I'm inclined to go without in the short term and wait for the PR to be merged and neostandard to be updated.
Cruikshanks
force-pushed
the
pretty-branch
branch
from
d6136d7
to
3acab74
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Cruikshanks
added a commit
that referenced
this pull request
Nov 22, 2024
When we merged [Use prettier to manage code style and formatting](#1476) we accidentally reverted changes we'd made to our JSDoc ESLint config to now error rather than warn when there is an issue. This quick change fixes the config.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DEFRA/water-abstraction-team#115
Now, we are a team of seven, with seven different opinions on the 'right' way to write the code! Because of our size, we are splitting into two teams to focus on various features simultaneously. But we'll still be working with the one code base.
We'd already moved from just using StandardJS to lint our code to using StandardJS via ESLint (we kept the rules but not the tool) because there are too many cases where StandardJS has no ruling, but we wanted one.
However, each time these rules don't provide a style convention, the team must stop, discuss, debate, and finally decide how something will be done. We want something else to take the decision away from us!
Step forward Prettier. Prettier is an opinionated code formatter that focuses only on the style of the code.
So, any rules or conventions we have that would affect the Abstract Syntax Tree (AST) would still be controlled by ESlint. These are the things as a team it is worth spending time debating and agreeing.
For the rest, we intend to let Prettier take over. It is widely used across the JavScript community and is popularly advocated for teams that become large or dispersed like ours.
Our approach to the adoption was first to remove our existing ESlint config, add Prettier, and then let it update all the files.
Then, having checked through as a team there were no 'red-line' changes, commit them. We then add ESlint back in and only re-apply the non-stylistic rules.
Our research found that JSDoc is still better managed through ESlint (Prettier does nothing with it), so we also added our original config for that.
Another fundamental change is that we are no longer bringing in StandardJS as an ESlint extension. This means we can move to the latest ESlint v9 and away from the deprecated config file format to the new flat file config.
Previously, we'd been blocked because extensions are not supported in ESLint v9. StandardJS does provide eslint-config-standard, but along with the core project, it does not appear to be actively maintained, is still using the deprecated ESLint style rules and has an error so cannot be used.
We'd resigned ourselves to manually copying and updating the rules from their plugin when we revisited an old issue when first switching to ESLint with StandardJS. Support for Eslint v9 Flat Config format was one of the blockers that meant to have both ESLint and StandardJS play ball, we were stuck on ESLint v8.
We spotted that there had been some recent activity, all of which referenced an alternative called neostandard.
And oh gosh! All our dreams were answered.
;is now an option, and disabling style rules for those opting to use prettier is possibleFor context, maintenance on StandardJS and related packages like eslint-config-standard has been stalled for some time. neostandard references the issue as being a block in governance and direction of travel. I've not been through every message, but it appears the maintainers are split between those who remained committed to StandardJS's 'one-tool one-way' approach and those looking to move to where most folks are: ESLint.
Even our own @johnwatson484 has gotten involved!.
We're betting this isn't going to be resolved any time soon, so to avoid us having to maintain standard rules in our ESLint config manually, the final fundamental change to highlight is we're now using ESLint + neostandard to manage coding rules.