Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move the codebase to 4 spaces indentation #2298

Closed
CarloLucibello opened this issue Jul 25, 2023 · 6 comments · Fixed by #2323
Closed

move the codebase to 4 spaces indentation #2298

CarloLucibello opened this issue Jul 25, 2023 · 6 comments · Fixed by #2323

Comments

@CarloLucibello
Copy link
Member

Two spaces indentation has always been an anomaly in the landscape of julia packages.
Should we get rid of it?

@ToucheSir
Copy link
Member

We're definitely long due for a formatting pass over the whole codebase + some tooling to ensure consistency going forwards. I think where things got stuck last time this came up was on which style to use. To my knowledge most of the presets in JuliaFormatter have received changes/bugfixes which make them more appealing now than before too (e.g. less aggressive line splitting of parameter lists).

@darsnack
Copy link
Member

In favor of this and adding JuliaFormatter in whichever style we all like the most.

@codetalker7
Copy link
Contributor

Hi. Do we also want to add some Git hook to enforce the formatting style after each commit (or something similar)? Or do we leave it to the users to run the Julia Formatter whenever they make some code changes?

@ToucheSir
Copy link
Member

A pre-commit hook that either checks or auto reformats would be ideal, just not sure how to make the installation and setup process for that straightforward. We'd also want some form of defense in depth for edits which are made e.g. using the GH web editor. There we can probably borrow the formatting check actions config from another Julia package.

@codetalker7
Copy link
Contributor

codetalker7 commented Aug 26, 2023

@ToucheSir, more questions: are we fine adding JuliaFormatter.jl to our deps? In that case I think we should be able to make a pre-commit hook that automatically runs the formatter for our given configuration.

Another option is to create a new folder (say .dev) which has it's own Julia environment with JuliaFormatter.jl being a dependency. Then we can run JuliaFormatter from this environment with our config file (ClimaCore.jl has such a folder, but I'm not sure if they have a hook).

Also, there's another alternative to a pre-commit hook: how about just adding a test in CI which checks whether the code has been formatted as per our style? Something similar to the config in ClimaCore.jl: https://github.com/CliMA/ClimaCore.jl/blob/main/.github/workflows/JuliaFormatter.yml?

@ToucheSir
Copy link
Member

JuliaFormatter shouldn't be in the deps. I think a separate environment for tooling would be required as you say, though I'm not sure it needs to start with a ..

Also, there's another alternative to a pre-commit hook: how about just adding a test in CI which checks whether the code has been formatted as per our style? Something similar to the config in ClimaCore.jl: https://github.com/CliMA/ClimaCore.jl/blob/main/.github/workflows/JuliaFormatter.yml?

We'd need both, ideally. Having just the hook would miss contributions made through the web UI. Based on what I've seen with other packages, having just the CI check has in the past frustrated contributors who don't aren't running the formatter locally at a repo level when they're forced to revise the PR just to fix those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants