-
Notifications
You must be signed in to change notification settings - Fork 13
chore: configure neostandard + prettier #710
Conversation
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/standard@17.1.2 |
Upgrade from `standard` to `neostandard` + `prettier`. Rework npm scripts for linting to be consistent with other repositories. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
bd7912c
to
92e448e
Compare
I updated the Prettier config based on our discussions above, removed settings where we use Prettier defaults and kept only settings where we differ. The new Prettier config: semi: false
singleQuote: true After reviewing the changes, I added two more files to
I consider this PR ready for final review & merging. |
Can we instead //ignore that one line, and lint the rest? I don't like this special case. You can make the same argument for other files with arrays. Instead, should we tweak the config to always list array elements on multiple lines? |
To my best knowledge:
I see two conflicting requirements here:
IIUC, avoiding special cases is more important to you than avoiding noisy diffs. Is that correct? I don't have a strong opinion myself, I am going to remove |
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
See 9c67ebb |
Have you looked into the other option I suggested, @bajtos? To configure the linter to always multi-line arrays, so that we avoid all of these cases? |
Yes, I did. Quoting from #710 (comment), with added emphasis:
You can find the list of all Prettier options here: |
Oh sorry I missed that. 😅 There's https://www.npmjs.com/package/prettier-plugin-multiline-arrays as an option, if you agree I'm happy to add it to this PR. Btw, I think we should soon think about bundling our linting logic into a module, so we can share the configuration among all our repos, and get dependabot updates for them. I'm also happy to work on that |
Nice find! If we are going to add prettier plugins, how do you feel about adding the following plugins too? (Discovered via https://github.com/electrovir/prettier-plugin-multiline-arrays?tab=readme-ov-file#compatibility):
Great idea! I propose the following next steps:
I propose creating two repositories for the shared config:
Thoughts? |
+1
+100
https://www.npmjs.com/package/prettier-plugin-jsdoc +100
💯
I had assumed we can create a binary, so it's just {
"dependencies": {
"@space-meridian/lint": "0.0.0"
},
"scripts": {
"test:lint": "space-lint"
}
} This way, the project doesn't need to bother and maintained tool versions (prettier & eslint). I do wonder about whether we need extensibility of the configuration, and therefore the approach you suggested works better. Wdyt? |
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Unfortunately, that does not work with IDEs like VSCode and Zed. There was a long discussion between Neostandard and Eslint maintainers on this topic, see e.g. neostandard/neostandard#2, neostandard/neostandard#33 and other linked issues/PRs. |
I am not able to get this working. Let's leave it up for future iterations. |
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
I added a bunch of commits to configure additional Prettier plugins:
For each plugin, there are two commits - one with the configuration and another with the formatting changes. I hope it will make it easier to review this pull request. |
Stop running linters for every Node.js version we support. It's enough to run linters on a single Node.js version. This should fix the CI failure in #710 caused by a linter crash on Node.js v18. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
The linters are crashing on Node.js v18, I created #714 to fix this issue. |
FYI, for me, I usually don't review individual commits, as you can't add comments. With the full PR view some things are a bit harder to see, but I prefer that approach for any PR that isn't millions of lines. |
Stop running linters for every Node.js version we support. It's enough to run linters on a single Node.js version. This should fix the CI failure in #710 caused by a linter crash on Node.js v18. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
I believe you can add comments to individual commits. The trick is to view the commit in the context of a pull request, e.g.
If you view the commit standalone, then yes, comments won't show in the pull request:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing commas woo-hoo! 🎉
Overall this looks very good to me. I don't have any strong opinions on how the code is formatted now but I'm glad we have settled on styling configuration. 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This is a follow-up for #710 where we switched from standard to neostandard. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
This is a follow-up for #710 where we switched from standard to neostandard. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
I copied eslint and prettier configuration from https://github.com/CheckerNetwork/assert-ok-response
I split the changes into two commits to make this pull request easier to review. The second commit contains linting fixes automatically generated by eslint and prettier.
Last but not least, I also reworked npm scripts for linting to be consistent with other repositories.
npm run lint:fix
Links: