Use an automatic code formatter

[manuelma]

We discussed using a code formatter to unify our style, @Tasqu mentioned https://github.com/domluna/JuliaFormatter.jl ? It seems like it's the way to go. Any opinions?

[manuelma]

I experimented with JuliaFormatter a bit and results are a bit underwhelming. The default is to break lines after math signs (+, -, *, /, etc), not before as we want it. Same applies to for and if keywords in generator expressions. So the result is more difficult to read compared to what we have now. Customization is possible by defining a custom style and then the methods that do the formatting, but it's not straitghforward and poorly documented.

[manuelma]

I posted an issue in JuliaFormatter: https://github.com/domluna/JuliaFormatter.jl/issues/683#issue-1572081395

[Tasqu]

Yes, I remember there being some slightly annoying styling differences that took a while to get used to. I never bothered to learn to tweak the formatter, and I'm not sure if it's worth it if its not straightforward to set up.

[clizbe]

Sounds like this was decided against. (Although it would really be nice.)

[manuelma]

Sorry @clizbe - we need this issue, the problem is we don't have the resources yet, but it should be possible to develop a custom SpineOpt style using JuliaFormatter.jl.

[clizbe]

I'm glad you say that. It really hurt to close it. We've done with together with eScience for our other model, so maybe we can figure it out.

[clizbe]

@manuelma Although is it really In Progress or still a ToDo? I gave it a "Soon" priority and it'll be on our radar - but I like to reserve In Progress for things being actively programmed this week.

[clizbe]

From our CONTRIBUTING:

• Include spaces
  • After commas
  • Around operators: =, <:, comparison operators, and generally around others
  • But not after opening parentheses or before closing parentheses
• Use four spaces for indentation (test data files and Makefiles excepted)
• Don't leave trailing whitespace at the end of lines
• Don't go over the 119 per-line character limit
• Avoid squashing code blocks onto one line, e.g. for foo in bar; baz += qux(foo); end

[clizbe]

@abelsiqueira Can you help with setting all this up so we get everyone on one page and don't have conflicts?

[manuelma]

That list is not exhaustive though - there are other things. I guess the final test is to run the code formatter and find minimal changes with respect to the current formatting.

[abelsiqueira]

Before setting the formatter, how do you want to run it? The traditional ways are:

• Manually - which I discourage
• As an automatic fix after PRs after merged - which is what many people do and I have done in the past. The disavantages are having one extra PR after most PRs. Since Julia did (does) not have a strong tooling ecosystem, this was an advancement on the manual run
• Automatically with the pre-commit - which is what we do.

If you use the pre-commit version, it can also be used to as a GitHub action to check that everything is working.

PS. In Tulipa, we use pre-commit and we additionally format and lint other files (markdown, yaml, etc.), and do some other "fun" things (e.g., check broken links in markdown).

[abelsiqueira]

A few of these required formatting options are not options in JuliaFormatter, but default. But since we also use some basic pre-commit options and EditorConfig, I am not sure. I think the following is the minimum close to what you need:

indent = 4
margin = 119

And then also add a EditorConfig config to enforce some of the others in the editor (below is copied from Tulipa)

# https://editorconfig.org
root = true

[*]
end_of_line = lf
insert_final_newline = true
charset = utf-8
indent_size = 4
indent_style = space
trim_trailing_whitespace = true

[clizbe]

Before setting the formatter, how do you want to run it? The traditional ways are:

• Manually - which I discourage
• As an automatic fix after PRs after merged - which is what many people do and I have done in the past. The disavantages are having one extra PR after most PRs. Since Julia did (does) not have a strong tooling ecosystem, this was an advancement on the manual run
• Automatically with the pre-commit - which is what we do.

I like pre-commit - we'll just need to set it up and have instructions for the group. @manuelma what do you think?

[manuelma]

In Toolbox each dev runs it manually. We considered the pre-commit at some point, not sure why we didn't go with it? @PekkaSavolainen @soininen do you recall? Or have any thoughts here?

[soininen]

We considered the pre-commit at some point, not sure why we didn't go with it?

I do not recall any discussion or decisions on the topic. I thought it was left for the developer to turn it on of off but now I see we have the .pre-commit-config.yaml file in Toolbox repo which turns pre-commit formatting off for everybody.

Personally, I like the pre-commit hook option. However, some developers may dislike it because it forces you to commit again if any code was formatted, or at least that is what I remember. Perhaps it just needs some getting-used-to?

[abelsiqueira]

You can have the .pre-commit-config.yaml and it doesn't enforce or prevent usage of pre-commit. However, if you use the pre-commit tool in the Lint.yml workflow as suggested above, then the PR will complain when the format is not passing. Thus, users that don't use pre-commit locally might have failing PRs every now and then. The pre-commit will indeed force you to commit again if the hooks don't pass. I would also recommend enabling the "Format on save" option on VSCode in that case to decrease these occurrences. That being said, normally if the pre-commit fails for formatting reasons, you can immediatly commit again with the same message, so you only lose a few seconds.

[clizbe]

I think I read somewhere that we don't like to leave things type unstable (is that right?). If so, this might be a nice option, since it would highlight whenever this is happening:

[clizbe]

@abelsiqueira I'm messing with the formatter, trying to find settings that most closely match our current style, but have a problem.

My Format on Save is ignoring my JuliaFormatter.toml.

If I start a Julia session and run:

    using JuliaFormatter
    JuliaFormatter.format_file("src/filename.jl")

It uses JuliaFormatter.toml.

If I change formatting in the file (like remove a space around an "=") and hit CTRL+S, it reformats it, but ignores the settings in JuliaFormatter.toml. -_-

@datejada has been trying to help but we can't figure it out. I just googled it, and it talked about the folder structure where JuliaFormatter.toml is located, but it's located in the same place as we have it in Tulipa, so: SpineOpt.jl\.JuliaFormatter.toml

[clizbe]

Using ALT+SHIFT+F also ignores the JF.toml.

[abelsiqueira]

What if you copy the make.jl from docs to src. Does it still behaves the same?

[clizbe]

Same behavior. Uses the .toml with using JuliaFormatter. Ignores the .toml when using CTRL+Shift+F or Format on Save. :/

[clizbe]

I think I read somewhere that we don't like to leave things type unstable (is that right?). If so, this might be a nice option, since it would highlight whenever this is happening:

@manuelma Is this something we'd like? If a field is untyped then it auto-formats to specify ::Any. Then we can see easier when it's happening.

[abelsiqueira]

I don't have any good suggestion. Monday we can check it.

[manuelma]

I think I read somewhere that we don't like to leave things type unstable (is that right?). If so, this might be a nice option, since it would highlight whenever this is happening:

@manuelma Is this something we'd like? If a field is untyped then it auto-formats to specify ::Any. Then we can see easier when it's happening.

Maybe the person who is working on fixing the type unstabilities can use that locally, but I don't think it belongs in the official source code so to say. Or at least that's my preference (::Any doesn't add much to the code's readability).

But it's very nice to know.

[clizbe]

@abelsiqueira I've gotten it almost "perfect" where it makes minimal changes to a sample. But besides the auto-formatter problem, I have 2 weird reformats that I can't find in the docs:

Any idea how to keep it from doing these? Don't waste your time if it'll take too much searching.

@manuelma Formatting choices that appear inconsistent in the code that we can still debate:

1. Whether to use trailing commas in lists - whether I specify True or False, it makes changes.
2. Whether to use ; before keyword arguments. We use them sometimes, but not all the time.

Right now, these are both set to "nothing" meaning the Formatter leaves whatever the user chooses, but if we choose something for consistency, the Formatter can apply it everywhere.

[manuelma]

Whether to use trailing commas in lists - whether I specify True or False, it makes changes

I think true is better (assuming we are talking of multiline lists of course). And yes, we haven't been really consistent on this one.

Whether to use ; before keyword arguments. We use them sometimes, but not all the time

This I think we can leave it as nothing? I'm not sure, is it possible to see what changes in our code if one sets it to true?

[clizbe]

For instance, in run_spineopt.jl it does this several times:

[abelsiqueira]

@clizbe, I don't know where the first one comes from, and I wish I did, because I'd like to change it too.

The second happen because the + is not an operator in that situation, i.e., it's like the + in x = +1 - 2, just telling the sign of 1, not performing any operations. (Unnecessary extra info: + 1 will compute +(1), i.e., a one-argument function called + that does nothing in this case, while +1 is understood as the number 1 directly.) Note that it would do the same for -. Solution is to accept, or remove the unnecessary +.

[clizbe]

@abelsiqueira I think I've figured out the settings and have reformatted src - but just remembered that my Format On Save is ignoring my JuliaFormatter.toml still.

Do you have time to help me with this (maybe in a call?) and with next steps for implementing the formatter for everyone? I think we need to announce it, as well as immediately make a new PR with pre-commit as well. (So separate PRs, but implemented one after the other.)

[abelsiqueira]

I have the time. Just need ~5 min to setup. Send me a Teams invite pls.