Formatting the code in Help files

[mtmccrea]

I’ve created a draft PR to reformat aspects of the code in the Help docs [#6306] (class help files only for now, not tutorials, overviews, etc.). This discussion space is for general comment/concerns about the idea. For feedback on specific changes, please comment on the PR directly.

[Meta: Why a Discussion on this? This may also have been an RFC, but RFCs seem geared for collaborating on a plan of action, while this is basically (mostly) done, given the current scope, and I wanted to just check here for any opinions that present yellow flags rather than sidetracking the PR. Maybe this is actually straightforward and the PR thread is enough...]

Background & Motivation

The general motivation is that the docs vary widely in quality and consistency of code examples, and this is a step toward cleaning it up. There are almost no syntactic changes, the vast majority of changes here are just handling whitespace. Here are some advantages of the change:

1. Carrying out the style guidelines makes the code generally more readable.
   Experienced users made lots of these examples, and their personal styles were imprinted in examples—it seems much of the “loose” formatting or compact style was convenient for them to type but not for users to read.
2. It sets an example for new users.
3. It makes it more likely that new contributions will follow the guidelines, and requests by reviewers to do so will hopefully feel less cumbersome.

Before releasing the PR for review I wanted to gauge any concerns people may have. One reason is because, while I don’t think this is a contentious move like the codebase reformatting, it does touch hundreds of files.

Luckily we already have style guidelines that (hopefully) make the changes fairly uncontroversial. But there may be outlying concerns and subsets of files that should remain idiomatic (Pattern classes come to mind) despite being less in line with agreed upon style, which was arrived at only relatively recently.

This was not a fully automated task, for better or worse. There was plenty of bulk replace on a file-by-file basis, one rule or partial rule at a time. I had my eyes on the changes at least 3 times: at the time of change, reviewing each commit (going through hundreds of files for each), then a final pass through it all again. In the final pass I caught ~3% error rate, so I’m fairly confident in the final result. That said, it’s likely a few mistakes slipped through.

I'm interested to hear of any thoughts or concerns in case I'm overlooking any side effects or need to consider organizational implications of such a large change (in terms of number of files touched).

[smoge]

It feels like a task for an automation tool. There is an unfinished project, and this would be a tour de force test for a source code formatter. It would be an opportunity to test its idempotence, robustness, etc. The downside is that it would mess with git history. The best approach might be to enforce the guidelines only on lines that will be changed.

EDIT: Please ignore. I thought you were referring to Class files, not Class helpfiles. In this case, it's 100% positive

[mtmccrea]

updated to clarify.

[mtmccrea]

@capital-G offered the following feedback on the PR, but I'll copy it here, because it could be a good thing to consider for a "bigger picture" update related to CI and the PR submission process.

One thing I would like to add to such an effort would be a way for future commits to automatically apply this style as well - that is the beauty of a style guide, no more discussion and work to enforce a certain style.

You mentioned that you used regex to identify and modify the existing code - we could add this as a check/script via a git hook, so that any new commits would have to pass this check/script as well, which would also automatically suggest/apply any necessary changes by diffing them with your staged files. The CI could also tell us the style guide status of a commit in this case.
Basically: before you commit code, a script will verify if the changes concerning .schelp files pass the "test" and if not, the necessary changes will be applied to the file by the script - you can then review them (diff your staged files vs your current files) and add them to your commit by staging the automatic changes to your commit. I can make a small video of this in case people are interested and it is common approach in other projects.

I suggested something like this in #6096, but maybe it makes sense to try again here?

If others think this would be a good thing to implement, I could provide the necessary code, but in this case we should bundle the "git-hook" implementation with these changes.

The regex criteria I used unfortunately still required lots of manual checking to catch the ~5+% of outliers for any given search. To get this to be fully automated is another task entirely. I know that "the great reformat" took a very long time and some tireless work to finally get all the rules tuned up in the linter.

Another point is that these changes are largely whitespace, so notably a few rules aren't included. These would be pretty high-impact changes, but I think tougher to implement, namely:
Rule: Use K&R style for multi-line blocks
Rule: Place the parameter list on the same line as the opening curly bracket of a function or method.
Rule: Place each element of a multi-line array on its own line.
Recommendation: Use trailing closure syntax whenever possible.

For a proper CI hook, these should probably be included.

That said, I'm admittedly not a regex wizard, so someone familiar with this is welcome to give it a go! I'll include the criteria I used in the final commit description for that ambitious person to use as a starting point :)

I can make a small video of this in case people are interested and it is common approach in other projects.

That would be very nice! Maybe that effort would inspire someone to slowly chip away at implementing it.

[capital-G]

That said, I'm admittedly not a regex wizard, so someone familiar with this is welcome to give it a go!

I would be willing to implement this if there are no objections to implementing this feature. I am not a regex expert, but I have enough regex experience to implement it.

[mtmccrea]

Sure, if you're up for it :)

if there are no objections to implementing this feature

I'd imagine this is tied more to your proposal for incorporating pre-commit git hooks?

[capital-G]

I'd imagine this is tied more to your proposal for incorporating pre-commit git hooks?

IMO, implementing a script that checks formatting, but does not automatically check and apply all future formatting as needed, will lead to more noise within the development process (aka apply formatting commits and reviews concerning "please run formatting to pass CI").
Personally, I found the easiest way to implement this is via pre-commit, and since we already have a Python dependency for C++ code formatting, we wouldn't really be introducing a new dependency and it would still be an optional dependency.

I will also try once more to propose switching to pre-commit for our C++ formatting needs once again, as this would allow us to drop (the currently unavailable with the proper version) clang-format as a dependency for formatting, see #6096 / #6196

But I am also interested in what other people think about this - especially if a unified style should be pursued - for now limited to the .schelp files - as I know there have been objections to this in the past, which IMO clash with the existence of a style guide.

[mtmccrea]

OK, I wasn't aware of the prior discussion about unified formatting for help files in particular. So I agree it's good to hear out opinions on that before putting in a bunch of work building the formatter.

If there was already consensus on style guidelines for the class library, extending those to help docs would be sensible, in principle. The guideline rules are quite few, I think intentionally, so as not to stifle some of the expressive SC "dialects". But, those rules were formed with the codebase in mind, not necessarily user-facing code (like examples).

My PR includes very few changes that "look like" style changes (see those rules I skipped above), which was perhaps convenient ;) And because I reviewed all the suggested changes in the process, I manually skipped over some occasionally if they were in an area that looked rather idiomatic (Pattern classes come to mind). So yea... I see how a full, automatic, and required formatting is a step or two beyond the current changes and warrant discussion.

[mtmccrea]

(the currently unavailable with the proper version) clang-format as a dependency for formatting, see #6096 / #6196

It would be great to follow through on that discussion. I agree it was good to make pre-commit a separate proposal from updating clang-format, the latter is an especially pressing need!