Style Guide and Style Checker

[willtebbutt]

In various other repos we make use of JuliaFormatter.jl + an action to enforce a standardised style guide for each PR. See e.g. KernelFunctions.jl.

Currently this repo doesn't seem to have this, but maybe we should?

[yebai]

Seconded.

[devmotion]

It should be possible to just copy the setup in DynamicPPL (which IIRC is slightly more recent and e.g. handles merge queues as well).

[torfjelde]

We indeed should. It's been put off because it will be quite annoying for the open PRs

[torfjelde]

So once we've closed the major ones that are currently opened, I'm very pro adding the same setup as in DPPL

[willtebbutt]

So once we've closed the major ones that are currently opened, I'm very pro adding the same setup as in DPPL

Sounds entirely reasonable. Which are the major open PRs? Would be helpful to have specifics on this I think.

[mhauru]

I support this and am happy to take on implementing this. We could also add this immediately with an ignore list covering all files currently under major work. Those files could then be brought into the formatting fold as PRs are merged. This way we don't need any one moment when all big PRs would be merged.

[mhauru]

JuliaFormatter is now running in #2255. I'm also exploring other tools. Some questions:

• Do people have opinions on JET.jl? I'm trying it out now to see if it would be useful, never used it before.
• Same question for Aqua.jl.
• Do people like using git pre-commit hooks? Scripts that run before making a commit and won't let you make a commit unless the script passes. I'll make one locally to stop me from committing code that doesn't pass the formatter. If others want to use it too I could make a script for it in the repo.

[willtebbutt]

Do people have opinions on JET.jl? I'm trying it out now to see if it would be useful, never used it before.

I really like using JET. I've incorporated it into my standard unit testing in Tapir.jl -- see here. It's definitely better than just using Test.@inferred.

Same question for Aqua.jl.

Not used it, but planning to start soon -- it's really nice that it hunts down obvious method ambiguities.

Do people like using git pre-commit hooks? Scripts that run before making a commit and won't let you make a commit unless the script passes. I'll make one locally to stop me from committing code that doesn't pass the formatter. If others want to use it too I could make a script for it in the repo.

I've not used them before, but I definitely like this idea. I can definitely see myself using one which checks for formatting!

[yebai]

Do people like using git pre-commit hooks? Scripts that run before making a commit and won't let you create a commit unless the script passes.

All of these are good improvements. I am not sure about pre-commit hooks since making them run stably across a wide range of operating system choices is hard. Github checks are good enough since almost everything goes through a PR.

[yebai]

Fixed by #2255 -- feel free to open new issues / PRs for JET and Aqua.

[mhauru]

Aqua seems useful, so I made a PR: #2257. I opened a draft PR to discuss pre-commit here: #2258. JET seems handy but I couldn't see an obvious way to run it wholesale over a package and have it be useful (many of the issues it raised seem to come from dependencies), so I left it be for now.

[torfjelde]

Do people have opinions on JET.jl? I'm trying it out now to see if it would be useful, never used it before.

Have only played around with it like a year ago with limited success, but given @willtebbutt 's experience it might be interesting to consider.

Same question for Aqua.jl.

This def seems like something we want:) Seems to strictly be an improvement without any downside 👍

Do people like using git pre-commit hooks? Scripts that run before making a commit and won't let you make a commit unless the script passes. I'll make one locally to stop me from committing code that doesn't pass the formatter. If others want to use it too I could make a script for it in the repo.

Against this because we have format checks on PRs. This lets you play around however you want when just working on a branch, but as soon as you want to make a PR, you'll need satisfy the formatting constraints. In particular for Julia, having to run formatting on every commit (which requires starting a separate script per commit, which has a delay that is much greater than in, say, python), unless you do something fancy with a server, can be annoying. Plus, in most cases, it's easy enough to set up "format on save" (e.g. in VS code or emacs). Aaaand we have automatic code suggestions from Julia formatter on PRs, so you don't even need to run the formatting locally 🤷

[devmotion]

I use Aqua (basically in most packages nowadays) and JET (in a handful of packages but it has been very helpful and I like it), so I think adding Aqua tests and gradually adding JET tests in cases where it is helpful is a good idea.

I also tend to think that a pre-commit hook for formatting might be inconvenient and the Github check should be sufficient (since everything goes through a PR).