Choosing a formatter for McStas

[Lomholy]

Sometimes when we make large components and work on them multiple people, the indentation gets mixed up. We use different coding styles and paradigms, which by themselves are fine, but when mixed they get confusing to read. This discussion is an extension of #2308 which showcased the default clang formatter.

I therefore propose that we choose a formatter that we accept to be used when working on mcstas components.

I propose that we either use the clang-format formatter or the astyle (Artistic style) formatter.

I will spend some time in the coming days, coming up with which settings could be used for a formatter, and showcase what this results in for a couple of different examples, namely the Monochromator_Bent components and the Union_mesh component.

Looking forward to hearing your thoughts on this!

[willend]

@Lomholy let me first indicate that I more or less agree:
Different-styled whitespace/indentation by different people can sometimes make commits and code changes quite hard to read.

Historically I have myself “lived with” whatever indentation the original author chose and tried to stick with that in the individual file - or if I changed the whitespace / indentation done that work in a separate commit.

History aside…
I think a reasonable solution would be:

• Agreement on a good style
• Create a tool to do the work on ALL comps / instrs in the code base

I have a prototype in place at https://github.com/willend/McCode/tree/clangformat-experimental, I simply asked an AI to:

Please write a tool for adapting all .instr and .comp files in github.com/mccode-dev/McCode to a recommended clang c-indentation. Only work on the code inside %{ %} sections. Please keep newlines after %{ and before %} (these separators should in other words stay on their own lines). Also, please treat the %{ and %} section delimiters as “first level code”, i.e. please indent the code within.)

I would prefer to stick with a “contributor/developer tool” and not automate via CI for now…

(The Windows CI-tests are completely overwhelmed by the sheer number of changed comps/instrs: find: The environment is too large for exec(). - but the Unix tests seem to be progressing OK. Once the ‘basictests’ are done I will try running the “testsuites” that include all instruments.)

[willend]

(… And if anyone was in doubt: I hate IDE’s/editors that auto-indents the code…)

[Lomholy]

@willend weren't you on vacation? ;)

Jokes aside, this is exactly the sort of thing I was hoping for.
The google style you seem to have set as a default seems like a solid choice.
However, when I look at your commit, It does not seem like that is what was applied to the code. (It seems that your simple indent formatter took over).

I agree with 4 space indentation, but I honestly dont think we should indent each section per default.
I tried using it sometimes but I ran out of screen space at every other if statement / for loop I wrote...

I really appreciate the 120 column limit. I agree that 80 is too small, but an upper limit is nice to have, just to be able to keep code somewhat together.

I think I will warn against using our own "homemade" auto indenter, as there are so many edge cases... so sticking to clang-format or another tool is probably better. Having the homemade solution as a fallback is of course fine for a start, but in the long run, it might end up hiding issues.

One thing I would add is to indent in preprocessor directives i.e #ifdef and others like it. It seems that clang-format has this under IndentPPDirectives.

(... And if anyone is in doubt: I don't mind auto indenting IDE's. That means I don't have to do it...)

[willend]

@Lomholy vacation also sometimes means peace and quite for ‘fun and hobbies’, for me a gray-zone that at times overlaps with actual work… ;-)

The branch mentioned above was by no means meant as ‘the solution’. Simply a demonstrator to outline how I would personally prefer this implemented. And this time round I experimented by ‘letting the AI do the coding’… (“Det er det de unge vil ha’…” ;-) )

I think it would be sensible to converge on:

• An easy-to-use in-repo devel/bin tool written in python
• To be thought of as a UI/frontend for clang-format which exists on is own as a conda-forge package or as part of clangdev

That said, my practical experience with using clang-format is at this point limited, but you have (had your IDE) run it I guess?
Can you share either the formatting-settings file you used - or even better the commands in practice executed?

• Further I am honestly more inclined toward two-space indentation to save some real estate. I would actually even prefer a legacy 80-char limit, as this makes the code more readable also on a small, old-school terminal and here on GitHub… But I sense that fight is perhaps already lost… :-/

• I further think that we should at least try to formulate a few common “rules of engagement” / common coding style habits. For me it is very important to avoid these ones...:

    // 1.
    my_own_very_long_typename_pointer* Variable_name_that_spells_out_too_much_Maybe_Even_caMeL_case;
    // 2.
    /* Commented section containing ‘dead code’, i.e. some random, earlier version of now production code existing elsewhere 
        in the file */
    /* my_own_very_long_typename_pointer_OLD* Variable_name_that_spells_out_too_much_Maybe_Even_caMeL_case_OLD; */

• Specific parts of the codebase has a lot of that - which to me makes it utterly unreadable.

---

<rant>
About auto-indentation I have to insist that a lot of the mishaps of mixed code structure in various parts of the code-base came about from exactly the use of auto-indentation…

To me whitespace is not just whitespace - and sending commits that change other users code drastically at the structural level MUST happen independently of actual code/algorithmic changes. Otherwise later effort to disentangle / deconstruct a code issue becomes so much more time- and energy-consuming...
</rant>

---

[Lomholy]

@willend The tool actually worked pretty well, which I suppose is why I gave my input.

Tinkering a little with it made it use a .clang-format file which I have defined. I ran it for all the instruments and components (I think).
This can be seen in the format tool test branch.

Regarding the indenting, I am not much of a fan of two space (I do actually tend toward the old guide of "if your indentation is making your code wide, then you are indenting too much.")

I also tend to agree with you on the 80 char limit, but with the codebase in its current state, I think it would make the code more unreadable (in part due to the long variable and struct names used), than if we stick to a 120 char limit.

Regarding the coding styles, I completely agree with your two suggestions, but noting that they are guidelines, not absolutes, as all code guidelines are.

One thing I would like, but I think is very much against everybody elses opinion is this: Functions should generally fit unto a single printed page in your normal font. This usually makes your function keep a red thread, and forces you to split up the code into smaller, more understandable bits. I would argue that TRACE should also be such a function, albeit I know many disagree (or at least the written code disagrees.).

<rant>

Both those auto indenter issues are solved if you, inside a project, agree on which formatter to use.
For an instance I fully intend to implement the formatter we agree upon here, into the VS code extension, essentially forcing it unto users that "don't know better".
</rant>

[willend]

@Lomholy thanks for playing with the tool and adapting, let’s see what we will eventually arrive at.

Tinkering a little with it made it use a .clang-format file which I have defined. I ran it for all the instruments and components (I think).
This can be seen in the format tool test branch.

Hmm maybe, I will see if I can figure that out. :-) The issue is that the *basictest* yml/bash-scripts easily get overwhelmed with bulky code changes… The best way to ensure that all instruments function correctly is by executing the two workflows mcstas-conda-testsuite and mcxtrace-conda-testsuite on the given branch. (Windows on mcxtrace-conda-testsuite will fail until #2192 is eventually solved.)

Regarding the indenting, I am not much of a fan of two space (I do actually tend toward the old guide of "if your indentation is making your code wide, then you are indenting too much.”)

You are mentioning two space - not four space, was that a mistake? It seems so if you are at the same time making the argument that "if your indentation is making your code wide, then you are indenting too much.”? :-)

I will have to insist that we need indentation inside the %{ %}’s as these will always become part of a generated code, either as part of a struct or function. Remember: We would also like our generated code to be readable as far as possible. So we will have to teach both of your clang-format profile and the script to do that...

Both those auto indenter issues are solved if you, inside a project, agree on which formatter to use.
For an instance I fully intend to implement the formatter we agree upon here, into the VS code extension, essentially forcing it unto users that "don't know better”.

Whatever we end up agreeing I think ought to become available both within the devel tree as e.g. a .py script and as (for instance) a VS code extension. This gives room for both “old dogs that have a hard time learning new tricks” and the “youthful youth and their AI-driven IDE approaches”. ;-)

[willend]

Heh, one more observation:
Once we arrive on “something” I think we will have to:

1. Add the tool / the clang formatter alone to main in a dedicated PR
2. Add component edits to smaller portions of the code-base at a time (e.g. one comp category pr PR)

I simply can not wrap my head around these big commits that contain edits to 1000 files… :-)

[Lomholy]

Hihi I agree. They are way too humungous. Sounds like a good plan!

[Lomholy]

Regarding the indenting, I am not much of a fan of two space (I do actually tend toward the old guide of "if your indentation is making your code wide, then you are indenting too much.”)

You are mentioning two space - not four space, was that a mistake? It seems so if you are at the same time making the argument that "if your indentation is making your code wide, then you are indenting too much.”? :-)

My intent is that I enjoy four space, since four space makes you see the indentation more clearly. The argument against four space is of course real estate, where I mean that if you are lacking space because you have indented, you have indented too much.

But maybe we should go with two spaces if we insist on indenting inside sections as default.

Hmm maybe, I will see if I can figure that out. :-) The issue is that the basictest yml/bash-scripts easily get overwhelmed with bulky code changes… The best way to ensure that all instruments function correctly is by executing the two workflows mcstas-conda-testsuite and mcxtrace-conda-testsuite on the given branch. (Windows on mcxtrace-conda-testsuite will fail until #2192 is eventually solved.)

This will be solved if we do the formats one category at a time, if I understand the issue correctly?

Whatever we end up agreeing I think ought to become available both within the devel tree as e.g. a .py script and as (for instance) a VS code extension. This gives room for both “old dogs that have a hard time learning new tricks” and the “youthful youth and their AI-driven IDE approaches”. ;-

I wholeheartedly agree.

[willend]

This will be solved if we do the formats one category at a time, if I understand the issue correctly?

Only partly, what will be solved is the “overwhelming” ( or at least some of it).
The other issue #2192 is missing work on complex numbers in McXtrace courtesy of MSVC… :-| :-)

[Lomholy]

Oh microsoft....
How lovely thou art

[willend]

They have implemented only "part of” the C std99 complex number specification… You can’t use e.g. * for multiplying or + for adding…. So we now have mccode-complex-lib.h as ‘glue’...

[willend]

@Lomholy script-code only branch is now at https://github.com/mccode-dev/McCode/tree/formatter-tool-only with minor tweaks.

Do you have required syntax to allow the initial 2-char indentation within %{%} blocks?

[Lomholy]

Not right now, will make it soon

[Lomholy]

Updated the dev tool to do the initial 2 char indentation within %{%} blocks.

I have currently noted 2 issues with the formatter:
% include lines are being shifted around.

And also, using the formatter on Union_mesh.comp yields a strange indentation for the Finally section

[willend]

Cool, will check out the issues relating to %include and the Union_mesh to see if I can figure out a way around / what is happening. (I suppose it is a matter of regex “massage”.)

[willend]

@Lomholy I am quite happy with how our little prototype performs now - and have a “solution” in place to avoid clang messing with the %include lines…

[willend]

If you feel like it, I suggest you try taking a look at (some of?) the new branches where I have tried applying the tool? :-)

(I myself think it looks mostly OK - but will have to look properly later. :-) And no edits done to mcxtrace or share files as of yet…)

[Lomholy]

It does look mostly okay - I think it may be a little agressive in line wrapping, see Union_master line 422, and I don't really know what the good solution for Preprocessor directives is. To my mind,

#ifdef
line
line
line
#endif

Should be

#ifdef
line
line
line
#endif

But I can't quite see how to set it in the yaml file.

[willend]

I think it is fair to require that for “long comments”, these should be on separate lines, i.e. manually reformat like this:

  // Starting with 1 as the surrounding vacuum is considered a volume
  number_of_volumes = 1;

For the preprocessor directives I guess the “solution" is the same as I used in the script for %include within %{%}’s see around line 133… I.e. skip that part of the formatting in clang-format but do it manually in the tool… (Which can then in principle be run on the CI-side, pre-merge or something like that…)

[willend]

The above “solution” for #define and friends is in the tool since last night. Will do a fresh (set of) branch(es) to potentially import the code-edits. Next will be looking at implementing a CI-check based on the tool. (Error if there is “work to do”.)

[willend]

Hmm. More work / adaptation incoming…

[willend]

I think I’ve found a good solution (admittedly not in the .clang-format file only) to handle both of % and # within %{… %} blocks:

• The py script comments such lines (with a specific comment-token //__ESC__PRE)
• Calls clang-format for the indentation treating the above lines as comments
• The py script lifts the comment-token again

Also, I found that clang-format did not actually honour the config file when located in devel but it does when it is located in the base McCode dir as a dotfile. In the process I changed indentation style to GNU and also increased the number of columns to 160… (sometimes the indentation failed dramatically, even leading to compilation errors)

We could then start a slower process to identify where very such long in-line comments occur, fix them and then later apply an 80 char column restriction and a ‘soft’ code of conduct formulation to prefer non-inlined comments.

Test output looks OK for all of the comp categories that I applied:

• monitors
• sasmodels
• union
• optics
• misc
• contrib

(And I am thinking I will not touch / then flush obsolete soon)

[willend]

If you agree this is a reasonable prototype @Lomholy I will then

• Open a PR to add the tool to main
• Add a note in the PR template / TODO list for component inputs
• Start working at a CI-mechanism to check if there is work for the indenter to do (then error / prompt the user to do that in-repo)
  …etc.

[willend]

@Lomholy impatient enough that I added the PR :-D

[Lomholy]

Hehe. Sorry about the silence, am at a course right now, but I agree that this seems a reasonable prototype!

[willend]

Tool prototype is now in place on main and PRs have been added for McStas non-obsolete component classes.

McXtrace work currently pending.

[willend]

@Lomholy @mads-bertelsen @g5t @tkittel @farhi @ebknudsen since I have heard no loud screaming or vivid protests I assume you are sort of OK with the ideas outlined above. :)

Hence my plan is to merge these McStas-oriented PR’s that all came through the test without issues:

Next steps will be

• A clean-up of McStas obsolete
• The same steps for McXtrace

Integration of the tool into the CI will probably wait for a somewhat longer while.

[mads-bertelsen]

Just looked through some components after they were formatted, I think it looks really nice!

Only thing I noticed was #ifdef #endif blocks are not indented, which would be nice, I see you are trying to find a way to do this.

With regards to line length, I am not in favor of limiting that. It does have advantages in discouraging excessive nesting, but it can also lead to some compromises on variable naming.

In terms of implementation with CI, I think it's important to avoid flagging contributions as errors if they do not conform to the formatting, but instead just convert it automatically. In extreme cases I guess that can introduce a bug, did you see any converting the existing components?

[Lomholy]

This proposal I agree to 😄

[mads-bertelsen]

It could be a requirement to use it in the short term, but I am afraid to create too many hoops to jump through for people who want to contribute. I would be in favor of having the CI do it automatically, I believe the scipp team have it set up that way. There is a potential for a bug introduced by the formatter, which is why I am interested to hear how many issues were encountered converting all the existing components?

[willend]

The CI can not do it automatically - an edit “on top of a commit”. (Hence will require yet another commit, not done with user credentials. Can not happen.)

What could be done is to add a pre-commit hook, i.e. a command that is prefixed to the commit, done on the users machine. (This is not CI, CI is what automation runs server-side.)

Since there was never previously any formalities involved with contributing code, I think this small tasks is (along with the other check-marks on the PR-template) an ABSOLUTELY REASONABLE REQUIREMENT.

( And since it is done as part of working on a PR, this is not entirely on the contributing user. All very fair. What I don’t want is to promise that we blindly accept “anything" in any shape or form. Quality Assurance and standardisation must happen at the point of import. )

[willend]

No “bugs” introduced with the script the current form. But had to tweak it by annoyingly long lines that were broken in ways that caused syntax errors. ( Yes I think line-lengths should be below 80 chars. Expressions become very har to read with loooong variable names across multiple lines IMHO. )

The remaining problem with the script in its current state is that a few components will get “continuous edits” on multi-line-macros… Indentation on indentation on indentation if the script is run again…

[mads-bertelsen]

A pre commit hook sounds interesting, that might make it easier!

I do like longer variables and lines, it's clearly a preference, but I am also good with reaching a compromise. Thought I would look at the line lengths of components before and after the current linting work was implemented. (The before was just the last pull i did, so not right before the linting, but probably close enough). These are in log scale to avoid hiding anything under the carpet!

First before the linting:

Then after the linting:

Its interesting to see that before the linting, there is a clear drop in number of lines above 80 for most of the categories, though after the linter it is somewhat smoothed out, for example in the optics category.
A lot of the code certainly fits below the 80 line limit, though for samples, contrib, (and no surprise) union there is a lot of use of longer lines. Looking at the distribution for contrib, I can see a 120 line limit making sense.