[WIP] Add KWStyle and AStyle to the build

[zapashcanon]

Hi,

First, I'd like to say that this is far from being finished.

So, as it's been said in #3071, I started following the current conventions.

I'd like to first talk about max. line length and space vs. tabs.

Current convention is to try to limit line length to 80 char. I didn't checked all files, but I found lines with 211 char. I think we should just stop breaking lines. I don't know which software are used by LMMS developer, but personally, I'm using atom or nvim and they both display too long lines in a convenient way, e.g. :

I think, nowadays this is not useful anymore as every text editor should be handling this for you.

Would like to hear what do you think about this.

Currently, I set the limit to 250. I think we could set it to something even bigger, just to prevent some abuse, like 1000 char. Or we could just disable this check.

Then, tabs. I don't see a single reason why to use tabs over spaces nowadays. I don't know a single editor who can't replace tabs by space for you when you press tab. Tabs leads to strange display depending on your editor, so we won't see the same thing depending on our editor etc. So, I'd like that we switch to a 4 space indentation.

I'd like to fix those two points first, because they're leading to troubles with other checks.

To test the current KWStyle config, from the lmms folder:

KWStyle -v -xml KWStyle.xml -d src/core

[Spekular]

Conversely, I don't see a single reason to use spaces.

[zapashcanon]

Conversely, I don't see a single reason to use spaces.

Consistent display. Allow fine-grained alignment by tools like AStyle.

[Spekular]

1. Set tabs to show as whatever length you want.
2. Using tabs for indentation doesn't mean you can't use spaces for alignment.

[zapashcanon]

Set tabs to show as whatever length you want.

How do I set that on GitHub ? Plus, I don't want to set this for each project I'm working on, some are using 2 space tabs, some others are using 4 space tabs etc. And still, if someone set tabs=2 spaces, won't have the same than me with tabs=4 spaces.

Using tabs for indentation doesn't mean you can't use spaces for alignment.

Indeed, but it'll lead to error if you check for tabs indentation only. Or you allow tabs + space indentation, and it leads to a mess.

EDIT: moreover, if you don't want your editor to replace tabs by space, AStyle will do it for you, like all whitespace changes.

[tresf]

Tabs vs. Spaces

• LMMS thread Code Styling - Tabs Versus Spaces #3426
• Satirical video: https://www.youtube.com/watch?v=SsoOG6ZeyUI

I'm on the spaces side of the fence, personally, but I seem to be part of the minority (by a considerable margin). Reasoning is the the way spaces display -- It is is consistent among every editor on earth. Switching to 3 or 4 spaces is quite common in editors. Tabs generally consume 8 characters, which makes this whole "80 char width" problem even worse.

80 Char Width

Most developers are fine going over, but about 1/4 of our active developers still prefer to limit it to 80 due to display preferences. I know @jasp00 has stated he prefers 80 characters. I'm less concerned with the decision... it really must be abolished. I'm more concerned with how that will affect contributors like @jasp00. When we make this decision, we need to know how it will impact our contributors and that they have a valid workaround.

jasp00 said in #3071
@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.

Current conventions need to be changed. These tasks are really coupled. Unfortunate for @zapashcanon now his PR is going to be spammed with opinion. Hopefully we can keep this civil.

[zapashcanon]

his PR is going to be spammed with opinion

Actually, I don't like most of the conventions (about int* x vs. int * x and int *x, or { stuff). Fortunately, this won't be discussed in this PR. because this aren't necessary changes to add KWStyle and AStyle. :) So, after two two above, it will be fine I hope haha.

[tresf]

I don't like most of the conventions (about int* ... )
Fortunately, this won't be discussed in this PR

Well, now it will because you brought it up! :)

My vote is to put the modifier by the type, not the by the variable.

e.g.

Code	✅	⛔️
int * x	int* x	int *x.
int [] x	int[] x	int x[].

</opinion>

[zapashcanon]

Looks like we agree on this too. ;)

EDIT: @tresf maybe I should open an issue for each convention, and do some kinda poll in it so we'll be able to make some progress ?

[tresf]

and do some kinda poll in it so we'll be able to make some progress ?

We could but it's not a democracy. I feel the active contributors get to decide.

i.e. No offense to the community at large, but if they're not editing code on a semi-regular basis, their opinion really doesn't count.

[zapashcanon]

Would you agree on giving me a list of those contributors so we ask them ? Because, otherwise, I feel like this will quickly get stuck.

[tresf]

Would you agree on giving me a list of those contributors so we ask them ? Because, otherwise, I feel like this will quickly get stuck.

The broken CONTRIBUTORS file has a good list. Here's how we did it before we broke it. This only shows who's contributed, not how recent. Sorting commits by name would help us understand if they developer is long gone or not.

[BaraMGB]

The only thing I need are clear rules how to format the code. I don't prefer anything.

[zapashcanon]

@BaraMGB, maybe this wasn't clear in the first post, but anyway, you'll have something like: ./astyle.sh to do all the correct whitespace formatting changes for you. For the other changes, the CI won't pass if something is wrong with an error message to tell you the problem. Would it be ok for you ? We'd also update the wiki about coding conventions if they're changed I guess. :)

@tresf, I think I can find the list of contributors. ;) It's more like I don't feel like I should decide myself who is active enough to decide on this, as I'm not (currently at least) an active contributor. If you could clarify editing code on a semi-regular basis, it would help me. :) Or maybe, we could just let people decide if they consider themselves being active enough ?

[tresf]

Hopefully this is a good start...

• Wallacoloo, Lukas-W, softrabbit, jasp00, Umcaruje, BaraMGB, grejppi, michaelgregorius, zonkmachine, liushuyu, tresf

Some other recent contributors whose opinion would weigh in:

• karmux, Spekular, PaulBatchelor, follower, DeRobyJ, StCyr, zapashcanon

... and the ones that no longer contribute directly to codebase but would love to hear from:

• tobydox, diizy, Fastigium, curlymorphic, badosu

Left out because I assume little/no investment in this topic

• RebeccaDeField

(prob missed a few)

[jasp00]

I stick with my advice. IMHO, this is not the place to talk about conventions, even less about all of them at the same time. You should:

1. Indent with tabs.
2. Either wrap to 80 characters or not handle any wrapping.

[zapashcanon]

I stick with my advice

It was to stay as close as possible to current conventions, that's what I'm trying to do. All other things can stay as is, but those two can't imho.

even less about all of them at the same time

That's why I opened separated issues.

Indent with tabs

So it means no space indentation on line breaking. Which will just lead to have cases with something like 40 char. of indentation then 40 chars of real stuff, on this on many lines, making the code really hard to read. Moreover, nowadays, using spaces is what is the most used, see here.

Either wrap to 80 characters

So we have currently 1415 lines of code with more than 80 char. If we don't use spaces, it won't help to fix this.

not handle any wrapping

That's what I'm saying.

Still, I don't see any valid reason to use tabs or to limit line length to 80 char.

[Spekular]

Using tabs does NOT lead to misalignment because you can use spaces for this. If this creates errors then disable that check (because it's throwing errors for perfectly good code) or get a checker smart enough to allow alignment spaces.

[tresf]

Please move this to .travis. Also, I recommend making the filename lowercase, which is what we do for all non-class files.

[zapashcanon]

This is useless, I can just remove it if you want ?

Ok, I choosed this name because it was the one given as an example on the doc. Will do.

[tresf]

Either wrap to 80 characters or not handle any wrapping.

@jasp00 if you can live with no (mandatory) wrapping, we can make that call now and close out #3471.

[tresf]

Is it irony that this file uses 2-character indents? 😄

[zapashcanon]

Wasn't intended haha.

[jasp00]

if you can live with no (mandatory) wrapping, we can make that call now and close out #3471.

One issue is the coding convention and another one is what a formatting tool should handle.

[zapashcanon]

I'm closing, will do it another way.