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

Uniformize coding style #864

Closed
martonmiklos opened this issue Feb 25, 2020 · 10 comments · Fixed by #978
Closed

Uniformize coding style #864

martonmiklos opened this issue Feb 25, 2020 · 10 comments · Fixed by #978

Comments

@martonmiklos
Copy link
Collaborator

Hello folks,

I have found a few code style inconsistencies (whitespaces at the end of the lines, tabs, etc.) during my last contribution.

I would propose to agree on the coding style and document it in a clang-formatter format, format the whole code with it and setup a github action (or integrate the clang-formatter to the Travis CI) to check the coding style on each commit. I would also recommend to leverage coding style from existing projects to make it easier to define the it.

I know it will make the git blame harder sometimes in the future, however it is always more pleasurable to work on code which is formatted and indented consistently.

I can help with the github action setup, or even creating the clang formatter file.

@Nightwalker-87
Copy link
Member

Nightwalker-87 commented Feb 25, 2020

This is definitely a point that needs to be addressed. Though I suggest to push that until we have official contribution guidelines (where such styles should be defined), which I already spent some thoughts on. I can't promise these for the 1.6.1 release yet though I'd love to.

At the moment I am still reviewing old issues to verify that we don't miss any old unsolved problems from the past which we may carry with us without realising it. With this I also gain some knowledge about what happened in the recent years...

@bmarvo
Copy link

bmarvo commented Feb 25, 2020

I agree that we need to take a serious look at the style inconsistencies throughout the code base and I also agree we should push that out a bit until we can resolve some more of these old issues.

@Nightwalker-87
Copy link
Member

Nightwalker-87 commented Feb 25, 2020

I'll let you know as soon as I am done with reviewing the remaining 439 closed issues incl. PRs ... We will then see what remains and can then decide on a good point in time when to deal with this. I could imagine that such styling fixes in the codebase will result in many changes in several parts.

@Nightwalker-87
Copy link
Member

@martonmiklos: As you can see I've moved this to the v1.6.2 milestone for now, which is currently planned for September 2020 - so this can remain outstanding for quite some time. I assume that we will see quite some changes in v1.6.1, so it seems reasonable to wait with this in order to avoid (additional) incompatibilities.

@martonmiklos
Copy link
Collaborator Author

Alright!

@Nightwalker-87
Copy link
Member

I think we are ready to deal with this as from now.

@Nightwalker-87
Copy link
Member

I've attached this to a recent PR which also partially addresses this issue.
In fact this topic is being addressed in the background for a while already.
As of now quite a few source files have undergone a code style and white space clean-up.
Remaining files will be processed as soon as they are touched related to other issues.
Thus I think we can let git-automation close this ticket soon.

@Nightwalker-87
Copy link
Member

I would propose to agree on the coding style and document it in a clang-formatter format, format the whole code with it and setup a github action (or integrate the clang-formatter to the Travis CI) to check the coding style on each commit. I would also recommend to leverage coding style from existing projects to make it easier to define the it.

This is also going to be addressed in the linked PR.

@Nightwalker-87
Copy link
Member

Nightwalker-87 commented Jun 7, 2020

I came across the package uncrustify which can also be used along with the popular Atom-IDE integration atom-beautify (and I believe is supported by several other IDEs as well). As it is distributed as a separate package on several platforms, I'd consider that a favourable option to address the above topic.

@Nightwalker-87
Copy link
Member

I've added a local config file to the project and started going through all available options.
There are a lot of them that can be configured - an awful lot of options. 😓
I tried to break it down to the ones that seemed useful to me and I find this to work out really well per automation. With this, one can now just run such task every once in a while (e.g. when the visual appearance turns worse). Settings may also be modified at any time later on, if needed.
Altogether I believe this will surely save us a lot of time and efforts.

I'll push the related, commits soon, so you can have a look at it.
Details can then be discussed in the related PR.

@stlink-org stlink-org locked as resolved and limited conversation to collaborators Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants