Style & whitespace

[jameshcorbett]

No description provided.

[garlick]

This is a good idea IMHO!

However to get closer to project norms for C code you might want to pull in the latest attempt at a clang-format by @trws from flux-framework/flux-core#4694.

[trws]

Oh yeah... we should finish fixing that up. It was pretty close IIRC.

[jameshcorbett]

Should I hold off on this PR while we iron out clang-format or should I move ahead with it?

[trws]

TBH, I'd probably say go for it, we can always rerun later. Might be good to configure these commits to be ignored by blame commits in git though so they don't hide everything. Example here if you haven't done the .git-blame-ignore-revs thing before: https://akrabat.com/ignoring-revisions-with-git-blame/

[jameshcorbett]

OK done, although I noticed clang-format left some unreasonably long lines unchanged...

[trws]

Lines over 88 chars should only persist unbroken if there's nothing to break. Is there somewhere it's doing something strange?

[jameshcorbett]

No actually, upon further investigation there was one line that it left over 88 characters for some reason, and there were a few that I thought were 89 characters but were actually 88. All the changes are reasonable enough I think.

[garlick]

We normally break lines in C at 80 characters.

[trws]

We had that discussion before, probably in slack since it doesn't seem to be in the issue/PR history, when we were setting up black as the python formatter and I was working on clang-format the last time. The number of spurious linebreaks drops significantly if there's a little bit of slop space above 80, for black they chose 88, and at the time IIRC we had general agreement to adjust that, so that's what I had put in the clang-format as well.

[garlick]

Ah OK sorry! That's fine then I guess.

[jameshcorbett]

Whoops I had just adjusted it down to 80, will fix...

[garlick]

Compound conditional line breaks are usually done like the original in our code base. Is that fixable?

[trws]

Ok that's odd... I would have expected it to keep it the original way here because it's not an argument. There are other spots in this file where they got indented because there's nesting, but I'm not sure why this is happening. Will check it out.

[jameshcorbett]

This particular example probably no longer applies now that I brought the limit back to 88

[trws]

@jameshcorbett, what version of clang-format are you using? With these settings, and clang-format 16, I get this:

    if (flux_kvs_lookup_get_unpack (fut,
                                    "{s:{s:o}}",
                                    "execution",
                                    "nodelist",
                                    &nodelist)
            < 0
        || !(hlist = hostlist_from_array (nodelist))) {
        flux_log_error (h,
                        PLUGIN_NAME
                        ": "
                        "Error fetching R from shell-counting future");
        prolog_status = 1;
        goto cleanup;
    }

(note that this is without the wrapper I'm working on in the other PR to fix the dangling < 0 in the middle)

[jameshcorbett]

12.0.1

[jameshcorbett]

I'm updating it now...

[trws]

Good idea, technically 11/12 support everything in the config, but I think it was 13 that fixed a bunch of alignment issues when some of the options we use are combined.

[trws]

Do whichever you prefer for now @jameshcorbett, we'll hopefully hash out the last few bits in the PR over on core and probably have an update, but I don't see that as a reason not to use the tooling in here in the meantime. I want to avoid stalling you on that since we've been working on it, on and off, since 2015... hmm... what's the emoji for simultaneous laughing and crying? 😭 🤣 ?

[jameshcorbett]

Ok, sorry for changes, I brought the line length back to 88 since that's my preference.

[jameshcorbett]

One more push to update the clang-format used to version 16.

[jameshcorbett]

@garlick @trws OK to set MWP?

[trws]

Oddly, this seems to still have the alignment issue, I would have thought that would take care of it. it looks like the issue is from treating the _pack and _unpack functions as whitespace-sensitive. It's adjusting the indent of following || lines inward for some reason. For this project, since it doesn't use the "pair-per-line" jansson convention with those, removing the WhitespaceSensitiveMacros block from the config is probably a good idea. That should take care of it. Makes me wonder why I haven't seen that in the flux-core test though.

[trws]

That got the indents, it looks like it didn't show up in core because it's really, really rare to have pack/unpack in single if chains for whatever reason there. I'm good here pending others's approval.

[grondo]

The number of spurious linebreaks drops significantly if there's a little bit of slop space above 80, for black they chose 88, and at the time IIRC we had general agreement to adjust that, so that's what I had put in the clang-format as well.

For Python that is fine, but I do find myself lengthening shortening variable names to play the "how to get black to format this more reasonably" with 88 character limit. I wonder if we'll want to shorten it for flux-core at least. We have codified 80 character limit in RFC 7.

[trws]

Sure, not planning to argue that here so much as point out that's what's in the file James was pointed to. Could always do what the black, and spack actually, folks did and do our own test to determine where the dog-leg is between reducing extra lines and keeping things tight.

[jameshcorbett]

@garlick if you'll stamp this with your approval I'll go ahead and set MWP

[garlick]

OK I don't think these rules are ready for flux-core where there's a lot of established code purposefully formatted a certain way, but it does fix quite a lot here so LGTM.

[jameshcorbett]

Thanks!