Lmms: Integrate a code formatter into the build

[jasp00]

A code formatter, mainly for C++, should be integrated into the build. The formatter should follow the coding conventions, and the configuration file should become the reference. It should be able to format files (for developers modifying code) and check whether files follow the format (for the build process). Only a subset should be formatted, excluding third-party files.

Existing solutions (astyle, bcpp, indent, trueprint) should be explored before developing a custom tool.

[zapashcanon]

I was thinking about integrating astyle a few days ago as a first PR. I no one object it, I'll do it. :)

[jasp00]

I am not sure astyle should be used. It does not fix the issue, although it would deal with whitespace formatting, which is the most frequent case. Handling whitespace is the easy part, the tool should be able to detect header guards, order of includes, class names, variables, members, parameters and returns; it should allow extensions or hooks if it does not cover these cases. Are you sure astyle is the best option?

[tresf]

Putting the dev team on a single, solid IDE can solve all of these problems as well.

[midi-pascal]

Qt Creator? Lmms is a Qt app after all! :smile_cat:

[teeberg]

Maybe an .editorconfig file could do the job too. That wouldn't force us to use a single IDE but would let us use any IDE that supports reading these files.

[jasp00]

a single, solid IDE can solve all of these problems as well.

If there is an IDE that can do such formatting, we can extract the formatter.

[tresf]

If there is an IDE that can do such formatting, we can extract the formatter.

I'm talking big league stuff like CLion or a commercial IDE that actually helps do code simplification, formatting suggestions, standard helping. I'm not sure how Qt Creator compares from a standards perspective but I'd be happy to reach out and request a FOSS discount from a commercial IDE provider.

The reason I put emphasis on this is if we set an IDE standard, it will encourage video tutorials for getting developers involved, lower the barrier to entry and catch the problem as the code is happening, rather than at the time of the pull request.

Also, after a few PRs we'll find ourselves proactively fixing formatting (the IDE will yell at us if we don't), which is long overdue. I've never used CLion but I provide it as an example because I use a sibling product, IntelliJ daily and it has git integration as well as before/after diffs that are very helpful to see the red/green diff prior to committing, PRing.

I've personally requested discounts from JetBrains on sever occasions due to low budgets, etc and they've always worked with me on price. I'm sure other commercial IDEs will do the same. We have a small but adequate budget for this stuff too, BTW.

If a FOSS IDE can do these things great, but I've spent a lot of time with others (Eclipse, NetBeans) just to realize they break often and lack important features, ones that are worth the money when paid for.

Disclaimer: Most of my PRs are done with very limited text editors (TextWrangler, gedit, Notepad++, vim), so I'm no expert on C++ IDEs.

[zapashcanon]

@jasp00:

header guards, order of includes, class names, variables, members, parameters and returns

What do you mean by class names, variables, members, parameters and returns ? Checking if they follow camel case or our equivalent convention ?

I don't know any formatter like astyle (not IDE related, easy to use etc.) able do to this unfortunately.

@tresf, having to use a specific IDE for a project is the kind of things which will prevent me from contributing to a project and even using it... Just providing command line tools doing all the formatting stuff and let the people use whatever IDE they want is IMHO the way to go. If there's no such free tools, like @jasp00 said, extracting them from some IDE is all good, as long as they're open source.

[tresf]

@tresf, having to use a specific IDE for a project is the kind of things which will prevent me from contributing to a project and even using it... Just providing command line tools doing all the formatting stuff and let the people use whatever IDE they want is IMHO the way to go

"Providing" is a relative term here. If by "providing" you mean email people Travis-CI build failures, then what we're providing is quality control in the form of post-commit build tasks. This will work but will come 20 minutes after a PR is issued against our branch, not at time of coding, not at time of committing.

If by "providing" you mean we chain it into our build process... perhaps, however that still doesn't solve the problem at the root of its cause for the masses. Chaining warnings into the build process at least happens before the commit occurs, but now forces someone to tab back and forth between reading a terminal output and typing in a code editor. Better, but still not ideal.

@curlymorphic spent some time recording how to get QtCreator working with our project but there were steps missed which never got added to the video:

http://youtu.be/XTWnQPGL9xs
http://youtu.be/3OzGXfm6fqE

I think this is a good example of what we can provide to our devs. Assuming the IDE can support it, the IDE rules can catch the problems and we'll know there's some level of quality pre-PR.

To speak a bit more about my experiences, I use a JetBrains product for Java code quality and it's great. It catches unused variables, it auto-formats (spaces or tabs, whatever's used), it cleans up imports and even provides a very good inspector and live-debbuger which is very valuable when trying to track down a bug.

I just think that good code editors do this stuff proactively. If we can clone the rules for Travis, great, but I think if we can tackle the quality problems while also encouraging new devs, that would put the project on a more progressive track to address this particular issue.

P.S. @zapashcanon I don't recall saying "have to use".

[BaraMGB]

Qt creator's cmake support has simplified a lot since ver.4. You only have to open the CmakeList.txt. That's all.

[midi-pascal]

@BaraMGB :+1:

[zapashcanon]

So, I'd like to come back to this.

Here's my proposal:

• updating or defining new coding conventions (optional), I think some current conventions make the code hard to read sometimes
• using KWStyle in the build chain to check everything is fine according to our conventions - it covers a lot of things, I didn't find anything better after searching, see features
• using astyle to automate white-space formatting as it's not something people like to fix by hand - other things like member var name can be fixed by hand more easily

If you agree with this, I can open an issue to discuss coding conventions based on a proposal. Then I'll do one or two PR to add KWStyle and astyle.

(Note that KWStyle is only able to check things, not to fix them like astyle)

[tresf]

@zapashcanon I think that's a good start. Since we bundle 3rd party source code, it's important to have the ability to omit directories as well.

I think some current conventions make the code hard to read sometimes

I wholeheartedly agree with this, but I'll save my comments for the issue to discuss coding conventions. We'll need someone to be able to make the final decision though as we'll never reach a unanimous decision.

[zapashcanon]

it's important to have the ability to omit directories as well

Astyle and KWStyle allow this.

We'll need someone to be able to make the final decision though as we'll never reach a unanimous decision.

What about a vote on the issue ?

[jasp00]

@zapashcanon, I suggest you first implement a solution that follows current conventions as closely as possible and tell what rules you need to break. Other rules can be modified later.

[PhysSong]

Closing in favor of #3856.