Skip to content
This repository was archived by the owner on Jun 30, 2025. It is now read-only.

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Apr 2, 2025

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.

  • chore: configure neostandard + prettier
  • chore: npm run lint:fix

Links:

Copy link

socket-security bot commented Apr 2, 2025

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/eslint@8.41.09.23.0 Transitive: eval, shell, unsafe +86 10.2 MB eslintbot
npm/neostandard@0.12.1 Transitive: environment, eval, filesystem, network, unsafe +216 59.9 MB voxpelli
npm/prettier-plugin-jsdoc@1.3.2 Transitive: environment +36 1.77 MB hosseinmdeveloper
npm/prettier-plugin-multiline-arrays@4.0.3 environment +14 7.01 MB electrovir
npm/prettier-plugin-packagejson@2.5.10 Transitive: environment, filesystem +12 362 kB matzkoh

🚮 Removed packages: npm/standard@17.1.2

View full report↗︎

@bajtos bajtos requested a review from juliangruber April 2, 2025 09:09
bajtos added 4 commits April 4, 2025 08:23
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>
@bajtos bajtos force-pushed the neostandard-prettier branch from bd7912c to 92e448e Compare April 4, 2025 06:25
@bajtos
Copy link
Member Author

bajtos commented Apr 4, 2025

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 .prettierignore:

  • /lib/abi.json - this is an auto-generated definition of a smart-contract ABI we copy from elsewhere, IIRC.
  • /tsconfig.json - let's keep the list of included/excluded files formatted as one entry per line to reduce noisy diffs in the future.

I consider this PR ready for final review & merging.

@bajtos bajtos requested review from juliangruber and pyropy April 4, 2025 06:29
@juliangruber
Copy link
Member

/tsconfig.json - let's keep the list of included/excluded files formatted as one entry per line to reduce noisy diffs in the future.

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?

@bajtos
Copy link
Member Author

bajtos commented Apr 4, 2025

/tsconfig.json - let's keep the list of included/excluded files formatted as one entry per line to reduce noisy diffs in the future.

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:

  • You cannot add ignore directives to JSON files, because JSON does not support comments.
  • I could not find any config option to tell Prettier to use one line per array item in JSON files

I see two conflicting requirements here:

  • We want to avoid noisy diffs
  • We don't want special cases

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 tsconfig.json from .prettierignore

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
@bajtos
Copy link
Member Author

bajtos commented Apr 4, 2025

I am going to remove tsconfig.json from .prettierignore

See 9c67ebb

@juliangruber
Copy link
Member

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?

@bajtos
Copy link
Member Author

bajtos commented Apr 4, 2025

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:

To my best knowledge:

  • You cannot add ignore directives to JSON files, because JSON does not support comments.
  • I could not find any config option to tell Prettier to use one line per array item in JSON files

You can find the list of all Prettier options here:

https://prettier.io/docs/options

@juliangruber
Copy link
Member

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

@bajtos
Copy link
Member Author

bajtos commented Apr 7, 2025

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.

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):

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

Great idea! I propose the following next steps:

  1. Finish the discussion about the desired Neostandard & Prettier config in this pull request.
  2. Land this PR.
  3. Create a shared configuration
  4. Update this repo to use the new shared configuration
  5. Continue updating other repositories.

I propose creating two repositories for the shared config:

  • CheckerNetwork/eslint-config
  • CheckerNetwork/prettier-config

Thoughts?

@juliangruber
Copy link
Member

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):

https://www.npmjs.com/package/prettier-plugin-packagejson

+1

https://www.npmjs.com/package/prettier-plugin-organize-imports

+100

prettier-plugin-jsdoc

https://www.npmjs.com/package/prettier-plugin-jsdoc

+100

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

Great idea! I propose the following next steps:

  1. Finish the discussion about the desired Neostandard & Prettier config in this pull request.
  2. Land this PR.
  3. Create a shared configuration
  4. Update this repo to use the new shared configuration
  5. Continue updating other repositories.

💯

I propose creating two repositories for the shared config:

  • CheckerNetwork/eslint-config
  • CheckerNetwork/prettier-config

Thoughts?

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?

bajtos added 4 commits April 7, 2025 09:47
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>
@bajtos
Copy link
Member Author

bajtos commented Apr 7, 2025

I had assumed we can create a binary

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.

@bajtos
Copy link
Member Author

bajtos commented Apr 7, 2025

https://www.npmjs.com/package/prettier-plugin-organize-imports

+100

I am not able to get this working. Let's leave it up for future iterations.

bajtos added 3 commits April 7, 2025 09:59
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
@bajtos
Copy link
Member Author

bajtos commented Apr 7, 2025

I added a bunch of commits to configure additional Prettier plugins:

  • prettier-plugin-multiline-arrays
  • prettier-plugin-packagejson
  • prettier-plugin-jsdoc

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.

bajtos added a commit that referenced this pull request Apr 7, 2025
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>
@bajtos
Copy link
Member Author

bajtos commented Apr 7, 2025

CI / build (macos-latest, 18) (pull_request)Failing after 21s

The linters are crashing on Node.js v18, I created #714 to fix this issue.

@juliangruber
Copy link
Member

juliangruber commented Apr 7, 2025

I added a bunch of commits to configure additional Prettier plugins:

  • prettier-plugin-multiline-arrays
  • prettier-plugin-packagejson
  • prettier-plugin-jsdoc

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.

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.

bajtos added a commit that referenced this pull request Apr 7, 2025
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>
@bajtos
Copy link
Member Author

bajtos commented Apr 7, 2025

@juliangruber

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.

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.

https://github.com/CheckerNetwork/node/pull/710/commits/1c1ff2b0e751545587c8cfbb14a11447d112f16d

If you view the commit standalone, then yes, comments won't show in the pull request:

https://github.com/CheckerNetwork/node/commit/1c1ff2b0e751545587c8cfbb14a11447d112f16d

Copy link
Member

@pyropy pyropy left a 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. 👍🏻

Copy link
Member

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

love it

@bajtos bajtos merged commit be20315 into main Apr 8, 2025
19 checks passed
@bajtos bajtos deleted the neostandard-prettier branch April 8, 2025 15:26
Copy link

sentry-io bot commented Apr 10, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: Spark crashed via signal SIGBUS catchChildProcessExit(zinnia) View Issue
  • ‼️ ExecaError: Command was killed with SIGSEGV (Segmentation fault): /tmp/.f.fcs/usr/src/app/runtimes/zinnia/zinniad spark/main.js ?(zinnia) View Issue

Did you find this useful? React with a 👍 or 👎

bajtos added a commit that referenced this pull request Apr 11, 2025
This is a follow-up for #710 where we switched from standard to
neostandard.

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
bajtos added a commit that referenced this pull request Apr 14, 2025
This is a follow-up for #710 where we switched from standard to
neostandard.

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants