Improving maintainability: ESLint and Prettier #193
Conversation
…storybook-builder-vite-parent` package
… `.editorconfig`
* `cross-env`; * `eslint`; * `eslint-config-prettier`; * `eslint-plugin-only-warn`; * `eslint-plugin-prettier`; * `eslint-plugin-storybook`.
…from `format` to `lint`. Running `lint` and `lint-ci` scripts in production mode, for getting `console` and `debugger` warnings.
mrauhu
force-pushed
the
improving-maintainability/eslint-prettier
branch
from
12279cd to
22c3b98
Compare
Added `--max-warnings=0` argument for returning error with ESLint `only-warn` plugin.
|
I published a small portion of changes, and I got a lot of questions. Spoilers: Configuration of ESLint with Prettier plugins is based on the best practices from the Vue and Vite community. |
…y, but for sure)" This reverts commit 22c3b98.
|
It's looking very good so far! Code style PRs are always a PITA since everybody has so strong opinions about code style! I couldn't care less if it's 2 or 4 spaces or tabs or whatever, 80 characters, 120 characters, I just really like going with the default/recommended settings on everything. That's what I always do on greenfield projects. The trade-off in this case is for example the indent size, which will lead to merge conflicts in the other 10 open PRs, and git blame will be a bit less useful unless you ignore whitespace diffs (but only "power users" of the git tools do that, in my experience). |
Problem is, right now there is no code style, because in every file you can find different indent size, formatting, etc. After this pull request and next #195 is merged, you need run once for each pull request: Then commit and push changes. |
* `@types/node` for LTS version; * `@typescript-eslint/eslint-plugin`; * `@typescript-eslint/parser`; * `typescript`. # Conflicts: # package.json # yarn.lock
|
I checked out the branch, ran I believe these are due to long comments and strings which prettier does not break into multiple lines. There are also a number of parsing errors in .jsx files, like this: Lastly, there are a number of files which prettier wants to format but which are not being formatted by the current |
It's not just whitespace changes which will be made when we run formatting. I appreciate that the formatting is not being done in this PR, because if we do all of the formatting in a separate PR, we can use git-ignore-revs-fie to hide the formatting commit from git blame. |
Yes, because all formatting is done in #195. |
@IanVS I'm already working on the |
I'm just curious why we wouldn't want our IDEs to run prettier? It's an easy workflow which almost always ensures formatting is kept consistent. |
It's simple:
https://prettier.io/docs/en/integrating-with-linters.html Easy workflow which almost always ensures formatting is kept consistent:
|
If we want to start quoting the integrating with linters page we can't skip over:
Which mentions that eslint-plugin-prettier is not generally recommended with a list of downsides :) I'm still not convinced we shouldn't just make the traditional separation of Ealing handling code quality and prettier handle styling. |
|
@mrauhu Yes, you're using the config that disables eslint style rules, which is great. But eslint does not run on every file that prettier can run on. I think that's the main challenge here. I'm still unclear on why you prefer to run prettier via eslint. I haven't heard any reasons other than mentioning it's done in vue and vite, but maybe they have different needs. The downsides of this approach are:
|
|
I made all suggested changes. Please, merge it. |
I appreciate all the hard work you're putting into this, @mrauhu, and I hope that I'm not coming off as too demanding or nit-picky. But I would like to be sure that we are not causing trouble for ourselves down the line. @joshwooding and I both work in this repo fairly often, and we have experience working in other projects as well. In fact, I was one of the early maintainers of ESLint back around its 1.0 timeframe. So I think we are not speaking out of ignorance. I'm going to leave the final say here to @eirslett, since this is his repo. But here are my main concerns:
There are other decisions here which I don't agree with, but I have expressed them and don't care enough about them to press those points. But without the changes I've listed above, I won't be approving this PR, but I won't block it either, since like I said, I defer to @eirslett. Thanks again for all your work here, and your efforts to improve the codebase. |
|
@IanVS thank you for clarification. |
# Conflicts: # package.json
…` to `extends` array
mrauhu
force-pushed
the
improving-maintainability/eslint-prettier
branch
from
a8a5355 to
fdd592e
Compare
That's fine, we can live with a somewhat broken eslint config as long as it is temporary (for a few hours or days). Are there any blockers to merging this PR now, as it is? It looks complete to me. The only thing is that there are 28 commits (as of now) and many of these commits are simple reverts of one another, so the history is confusing. @mrauhu could you please squash all the changes into 1 commit, with a good commit message? Then we'll get this stuff merged! |
Right now there is nothing broken, I moved the ESLint config changes from #195 to #193. Aslo, I reorder commits between three pull requests:
I'm working on typization for #195 draft and soon convert it to pull request.
I can, but these pull requests are on top of each other, can you merge them one by one without squashing? |
|
@eirslett about squashing: You can enable the Allow squash merging option in the repo settings: |
I think history of commits provide a better understanding for a developer what is changed and why. It works great with an IDE, because you can view commit history for every file and line. |
|
Ok, there we go! Thanks a lot for the work! 🥳 |
Hello.
This pull request is part of #188.
Done:
package.jsonto.prettierrc..editorconfigwith the Prettier standard.@eirslett, @IanVS
Best wishes,
Sergey.