Coding style: doxygen page to rely on before contributing to doc

[fabien]

Hey there, just added in my branch a new coding styles page which reflects my modest understanding of what I read in the existing code, please advise so that I can submit a pull request with this this doc that should help future contributors not doing the same mistakes I did when adding documentation…

Please review here (criticism and add-ons are very welcomed!):

fab672000/tracktion_engine/blob/documentation/doxygen/src/tracktion_coding-styles.dox

[daniel]

If you want to mirror the coding style in Tracktion, I guess they will be quite similar to the juce coding standards (having both the same spiritual father):

Coding Standards | JUCE

[fabien]

Wow that’s great if we could agree to reuse this,
I’d love to add it in the tracktion doc, I added a src folder there and also of course in the doxygen config file, and would still need some buy-in on these in my documentation branch?
Any preferred folder structure name?

[fabien]

How about this now:

fab672000/tracktion_engine/blob/documentation/doxygen/src/tracktion_coding-styles.dox

[dave96]

I’m not sure how useful it is to copy and paste the contents of the JUCE coding style web page? Would’t just a link be the better option as if the JCS change that will automatically be reflected.

Also, I know a lot of people don’t like this but although we follow the JCS, this does allow some room for whitespace to be used to express things. Unfortunately this makes it difficult to use a tool like clang-format to auto-format everything.

Also in general, we don’t like PRs that go through the code base changing style. In an idea world we’d have perfect tests that will catch any subtle change in behaviour but the reality is we’re just not there yet (we are building out tests gradually as we touch areas of code and fix issues). We’ve been burned too many times in the past from “small style changes” causing subtle breaking changes for this to be worth the risk.

Tracktion Engine is a huge code base which we are constantly working on. As we work on different areas we modernise, refactor and comment. Because of this we’re also unlikely to accept PRs that change code without there being an associated test, either existing or new.

[fabien]

So it is not a copy and paste at all, the semantical content is the same, but the formatting is homogeneous with the doxygen documentation and a lot of tags were added.
Also, the link is there but you have to imagine what it could become with a table of content and “Getting started”, “Tutorials” pages and so on.
Once you navigate through, now it should make more sense than an isolated link from the documentation context, I hope.
Also, I think that already the code sections are easier to read with the doxygen formatting.

[jules]

+1 to Dave’s comment.

(I think when he mentions us having had problems with small tidy-ups breaking things, he’s probably thinking of stuff I’ve done …it’s fair to say I’m a pretty good coder, and I wrote a hefty chunk of this codebase… so if I can’t be trusted to always get “minor tidy-ups” right, you’ll understand why we’re very wary of taking that sort of PR from random people!)

While we do want to welcome enthusiastic contributions, it’s important to realise that every PR has a value/hassle ratio which mustn’t go below 1.0. If a PR adds a really amazing feature and is easy to review, then that’s awesome. But one that adds little value and takes ages to review + discuss can easily lose us money in terms of time and focus, and will cause grumpiness.

So there are two ways to get the ratio right:

• add some kind of amazingly valuable big new feature which we don’t mind spending time reviewing because it’ll be so great
• add minor improvements, but do so in a way that requires almost zero effort to review or discuss. That means the style, grammar, naming, tests, everything has to be obviously perfect to the point where we can just hit “accept”. Unfortunately this is really hard to do, because it’s impossible for us to explain everything that someone would need to know…

[fabien]

So I agree the value must be appreciated, the changes in the latest reviewed PR are all adding something IMO, as an example they fix all the annoying warnings in the doxygen doc generation except one on the tracktion_graph that I voluntarily left untouched for now.
@dave96 you have to review the changes in detail, they are now mostly changing the doc, the risk of regression is quite low for comment changes…

[fabien]

@jules

add minor improvements, but do so in a way that requires almost zero effort to review or discuss. That means the style, grammar, naming, tests, everything has to be obviously perfect …

After your review, I searched as an example for all the references to midi instead of MIDI in JUCE and there are plenty of them.

I guess my point is we never really like the code of someone else and that’s fine, but you need to be more objective sometimes about it, because it’s fine.

The value the latest PR adds is fixing all the documentation warnings except one that now allows developers to focus more on documentation real problems when they happen (serious bugs).

When you have a console page full of warnings ; it’s really hard to see what is really wrong…

EDIT: @dave96 About value, it also proposes a fix to crashing the doc when running the python script btw.
So for me those would be really nice to add, there is also another PR trying to fix the same bug, so it seems to matter for your users

[fabien]

@jules

it’s important to realise that every PR has a value/hassle ratio which mustn’t go below 1.0

Updated PR: removed the premature dox file add-on, removed the python debug addon that if is convenient is not really adding too much to the pr, removed the change to the doxygen file now needed anymore to limit the risk of regressions.