Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply clang-format on the repository #10622

Closed
wants to merge 47 commits into from

Conversation

patrickelectric
Copy link
Member

@patrickelectric patrickelectric commented Mar 14, 2023

Add clang-format configuration file, CI test and apply the necessary changes on code.
It uses the WebKit style (the base for the Qt style)

The idea is to merge before the next stable release and have a defined code style for the project for now on.

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
@patrickelectric patrickelectric added Dev Call A maintainer flagged for this discussion on our bi-weekly call Release: v4.3 labels Mar 14, 2023
LocationAvailable = 1 << 2,
AltitudeAvailable = 1 << 3,
HeadingAvailable = 1 << 4,
AlertAvailable = 1 << 5,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that these sort of changes make the code less readable not more readable. Is there some way to turn off the part that eats tabs?

@DonLakeFlyer
Copy link
Contributor

In taking a glance through I don't see this as making the code better. I actually see it as making the code worse. Also not too thrilled about taking a massive diff for this which will make going back through history to compare changes much harder won't it. I would much rather just enforce the existing qgc code style guidelines manually by review.

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Mar 14, 2023

There already is a defined coding style for QGC. Docs are in the repo: CodingStyle.* Those can be updated as needed.

@booo
Copy link
Collaborator

booo commented Mar 16, 2023

I think it's a good idea to enforce a coding style but I'm also unsure what the right moment is for such a change. It clearly makes merging/rebasing harder for a short period.

@booo
Copy link
Collaborator

booo commented Apr 5, 2023

Did we discuss this in a meeting? What do we do about this?

@DonLakeFlyer
Copy link
Contributor

I would argue that for open source where you have many different eyeballs coming and going looking at the code, that readability is more important than strict adherence to a coding style script. My disagreement with this is that it makes the code less readable. Specifically with respect to the removal of all tabbing for format.

@kibidev
Copy link
Contributor

kibidev commented Apr 11, 2023

I would argue that for open source where you have many different eyeballs coming and going looking at the code, that readability is more important than strict adherence to a coding style script. My disagreement with this is that it makes the code less readable. Specifically with respect to the removal of all tabbing for format.

Readability is a bit subjective in this case, I would prefer the new style as I am not a bit fan of alignment.
But, I think there should be a very good reason if you want to change the coding style of a project that is as mature as this one. The improvement we get in this case (and we there is disagreement if they are improvements) is in my opinion far too small to offset the disadvantages. Rebases/git blames/etc becomes much more hassle with changes as this.

A better approach, if possible, could be to define a clang format setup for the existing style, and enforce this for new code and changes only.

That said, style is important, so I think the initiative is good.

@julianoes
Copy link
Contributor

I would argue that for open source where you have many different eyeballs coming and going looking at the code, that readability is more important than strict adherence to a coding style script.

I would argue the opposite. Coming from other projects I am not used to alignment like that and it makes it harder to read for me personally. Also, doing drive-by commits and not knowing what style I should be using, automated formatting would be a relieve because I don't have to think about that. Besides I find myself distracted a lot by fiddling with spaces just to find that actually it's not consistent with the block above it.

That being said, I don't like the idea of the churn this would cause: for git blaming and forks.
I would only apply it file by file as files undergo significant changes.

And then I would clearly mark the style change commits and add them to the .git-blame-ignore-revs so that they can be ignored when git blaming. Also see: https://github.com/orgs/community/discussions/5033

@DonLakeFlyer
Copy link
Contributor

I would argue the opposite. Coming from other projects I am not used to alignment like that and it makes it harder to read for me personally.

If I'm in the minority with this alignment I'm fine with changing. Way back when it was mainly Gus and I who used the same style. That said there is so much style that follows this style already in the repo.

Is it possible to apply a style that doesn't collapse tabs to avoid wholesale changes?

@DonLakeFlyer
Copy link
Contributor

I'm closing this. Blasting a bunch of commits for spacing just doesn't make sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Call A maintainer flagged for this discussion on our bi-weekly call
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants