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

style: standardize whitespace and formatting #524

Closed
wants to merge 1 commit into from

Conversation

mtfurlan
Copy link
Contributor

@mtfurlan mtfurlan commented Jul 8, 2022

The two holy war topics this touches on is spaces vs tabs and if there
should be newlines around braces

omersiar suggested spaces in a github issue, and I chose no no newline
before open brace inside of functions, but not for functions.
I got this from the linux kernel style guide, and I like it because
functions set scope in C, while other blocks do not.

so for example:

void example(void)
{
    if (true) {
        doStuff();
    } else {
        otherStuff();
    }
}

Happy to change, but this is my preferences and it was inconsistant
so I felt I could choose.

Closes #518

The two holy war topics this touches on is spaces vs tabs and if there
should be newlines around braces

omersiar suggested spaces in a github issue, and I chose no no newline
before open brace inside of functions, but not for functions.
I got this from the linux kernel style guide, and I like it because
functions set scope in C, while other blocks do not.

so for example:
void example(void)
{
    if (true) {
        doStuff();
    } else {
        otherStuff();
    }
}
@mtfurlan
Copy link
Contributor Author

mtfurlan commented Jul 8, 2022

Oh also I put the uncrustify config I used into the repo.
To run this what I did was

for f in src/*.*; do uncrustify -c uncrustify.cfg --no-backup -l cpp $f; done

I dunno how we want to document that or if we even want to commit the uncrustify config.

@matjack1
Copy link
Collaborator

matjack1 commented Jul 8, 2022

hey @mtfurlan sorry but I'm against these massive changes, as they make the history more difficult to read :(

If we decide for a style I think it should affect all new changes, but these massive PRs don't add much (to me).

@mtfurlan
Copy link
Contributor Author

mtfurlan commented Jul 8, 2022

Hey @matjack1 thanks for the quick response.
I personally find mixing in style changes with function changes make history harder to read because then when looking at a diff there are two changes mixed together, some functional some style.
Commits like this do a single thing, so when you see it in history you know it doesn't modify the functionality so can ignore it and just git blame a second time or whatever.

Anyway I'm interested in trying to move towards a consistent style in whatever way you want.
Having a consistent style makes it far easier to work on the project because stuff like having a mix of spaces and tabs inside a single file is really annoying.

I'm just not entirely sure how to enforce style only in changed code because linters run on a file not on a section.
I can make a github action to run whatever we come up with and tell people when their PR needs style changes.
If that is the direction you want I can start playing with it and see what I can come up with.

A different approach would be adding a .editorconfig file, which might do what you want and make editors write at least the correct kind of indentation and end of file stuff going forward.
I haven't really used it yet so I can't say for sure but from what I've seen it's promising.

@matjack1
Copy link
Collaborator

matjack1 commented Jul 8, 2022

I know that you can go back in the history and dig, but it's annoying, then we get another thing like the last time: #449 which then rots.

We could auto format on precommit, to enforce one style automatically. Also the editorconfig approach looks fine.

In any case if we merge this I would like to avoid having every now and then these kind of big commits. I don't care about the style so long it's one and it's automatically enforced.

If you add some automatic formatting we can merge this :)

Also because I have a couple branches that will be a nightmare to rebase and merge if we merge this first :)

Thank you!

@matjack1
Copy link
Collaborator

I'm closing this as we are tracking the issue here: #518

@matjack1 matjack1 closed this Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants