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!

[mtmccrea]

The PR is approved and cleaned up now: #6306.

So I could either close this Discussion, or leave it open to take on the other issues raised by @capital-G (automatic formatting, git-hooks). What do you think?

[telephon]

I approved it because it only concerned class help files and it makes sense to unify the syntax here. The other issue is more complicated (maybe?) so it is ok to keep them separate.

[capital-G]

Yeah, I also think its best to keep them separated for now, though it would be nice if the non-squashed commits are still stored somewhere, it could be a good resource for future discussions about (automatic) styling of the docs/code.

I think we actually don't need to be too "strict" for the docs here - the docs should still be correct and not advocate "problematic/outdated styles", but it is actually beneficial if the docs show a variety of different valid styles within sclang as this will be common when interacting with SuperCollider code in the wild.
Although it is debatable if this variety should also be present within the classlib, as we officially have a styleguide for sclang.

The only thing where I would "enforce" a linter is on "critical" parts (e.g. CRLF vs LF endings) and/or if people start having discussions over code styles (e.g. this.callMe; vs this.maybe();^^) - linters/formatters should be used to make lives easier and ease discussions, not to create more :)

[smoge]

I approved it because it only concerned class help files and it makes sense to unify the syntax here. The other issue is more complicated (maybe?) so it is ok to keep them separate.

Good morning, comrades

Since the discussion regarding code comes and goes and has no particular place. (I think yesterday I talked abvout it with @capital-G) I'd like to point out some impressions I have regarding this topic and how the sc project could improve in some respects. (Feel free to move my note to another place, not problem)

SuperCollider sclang Coding Guidelines

Programming Style

• Stick to the existing coding style of the file you're editing. If you need to make stylistic changes, commit them separately from functional changes to keep the change logs clear.
• Avoid reformatting the entire core library. This practice often leads to unnecessary conflicts in the version control system, making it harder to track changes and understand the history of a file.

Naming Conventions

Lexical conventions:

• SuperCollider generally uses CamelCase for naming, 4-spaced tabs for indentation, etc. There is no significant discrepancy between source files.

Lucidity Over Brevity

• Write clear, transparent code rather than short, obscure code. Clarity is key. Write the code for humans, not just for the interpreter or the computer.

Notes

Consider why the SC codebase has so few comments. In other projects, where contributors are also high-level experts, comments are essential. Embracing this practice could significantly improve code readability and maintainability, benefiting both current and future contributors. Lack of comments may have contributed to minor issues over the years. For example, parts of the codebase that should have remained limited to C (like ABI) received changes using C++ features, creating problems for interfacing with other languages. This was manageable when one programmer designed the entire system, but it has since led to complications.

Some Basic Ideas

Significant comments should be set off by themselves with a heading in a standard form. For example (random example, it can be anything):

// Note [ParGroup usage]
// ~~~~~~~~~~~~~~~~~~~~~
// The ParGroup class is used here to create a parallel group of nodes in the server's node tree.
// Unlike a regular Group, nodes in a ParGroup are executed in parallel. This allows for concurrent
// processing, which can be beneficial for running multiple Synths simultaneously. However, it's 
// important to note that parallel execution does not always guarantee faster performance due to 
// the complexities and overhead associated with managing parallel tasks.

Refer to the note at relevant points in the code:

// See Note [ParGroup usage]

This ensures precision and longevity of the comments, avoiding vague references like "see the comment above."

Overview Notes

Overview Notes summarize how the system handles a particular topic. They should include:

• Topic: Identify the topic/problem.
• Design: Explain the overall design and the big picture.
• Moving Parts: Point to key bits of code that implement the feature, with specific references.
• Wrinkles: List any non-obvious aspects that need special treatment, with specific code references.

Being specific and using examples is crucial. Mention relevant tickets and test cases.

For comparison, please read this file from the core GHC code. See how comments are used and try to see its functions there:

See: https://github.com/ghc/ghc/blob/master/compiler/GHC/Core.hs

Cheers

[mtmccrea]

FYI @smoge I closed this discussion earlier because the feedback was that automatic linting/formatting would be best kept to a separate conversation/thread.

[smoge]

Ok, so I already gave my impression in advance. Thank you for both initiatives.