polybase / zk-benchmarks

https://zkbench.dev
40 stars 4 forks source link

ENG-1202: Add eslint rules #63

Closed timmyjose closed 1 year ago

timmyjose commented 1 year ago

Changes:

linear[bot] commented 1 year ago
ENG-1202 Eslint

Add the same eslint rules we have on explorer.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
zk-benchmarks ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 6, 2023 7:31am
timmyjose commented 1 year ago

(Moved the discussion here since that issue is marked as resolved now).

@calummoore So the default spacing in rustfmt is 4 spaces (which is what we want), and since we don't change any of the default values, we can probably eschew using an explicit .rustfmt.toml until we do.

I noticed some (very few) discrepancies when running cargo fmt on this repository as well as the Polylang repository. I will add an extra step (in the CI PRs for both repositories) for cargo fmt --check (using the default settings) in the CI for both repositories. Thoughts?

@mateuszmlc

mateuszmlc commented 1 year ago

cargo fmt --check in CI with the latest toolchain makes sense. As an example for when that matters, cargo fmt used to not support formatting the else branch in let Some(x) = x else { ... };, but new versions handle it correctly, so if a dev's toolchain was behind, cargo fmt on another dev's machine would format the code.

Also RE .rustfmt.toml, I think overriding rustfmt defaults is a bad idea, best to stick to the defaults

timmyjose commented 1 year ago

cargo fmt --check in CI with the latest toolchain makes sense. As an example for when that matters, cargo fmt used to not support formatting the else branch in let Some(x) = x else { ... };, but new versions handle it correctly, so if a dev's toolchain was behind, cargo fmt on another dev's machine would format the code.

Also RE .rustfmt.toml, I think overriding rustfmt defaults is a bad idea, best to stick to the defaults

Agreed. Especially about sticking to the vanilla formatting rules as much as possible. I'll update the workflows accordingly.

timmyjose commented 1 year ago

cargo fmt --check in CI with the latest toolchain makes sense. As an example for when that matters, cargo fmt used to not support formatting the else branch in let Some(x) = x else { ... };, but new versions handle it correctly, so if a dev's toolchain was behind, cargo fmt on another dev's machine would format the code. Also RE .rustfmt.toml, I think overriding rustfmt defaults is a bad idea, best to stick to the defaults

Agreed. Especially about sticking to the vanilla formatting rules as much as possible. I'll update the workflows accordingly.

Added cargo fmt --check for both the Polylang and zk-bench repositories in the respective PRs (issues ENG-1207 and ENG-1209).