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

Contribute: update style guide to google + (minimal) exceptions #1918

Merged
merged 7 commits into from
Jul 6, 2022

Conversation

tstastny
Copy link

@tstastny tstastny commented Jun 30, 2022

Updates style guide documentation with link to google, and out lines our exceptions.

@MaEtUgR @dagar please let me know if I got something wrong.

We would probably also want to add something about clang format / tidy once that's all up and running. But for now this at least gives the general direction what we're trying to move to.

Let me know what you think ;)

NOTE: clang format will be used where astyle was before, we will need to remember to update this doc with instructions for e.g. the hook setup and auto formatting make command (does it stay the same? make format ?) @MaEtUgR @hamishwillee
^see PX4/PX4-Autopilot#19855

en/contribute/code.md Outdated Show resolved Hide resolved
en/contribute/code.md Outdated Show resolved Hide resolved
Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

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

I had some minor comments. Otherwise it's good and like we discussed.
We need to transition with a good strategy. I agree that having the rules should be the first step. We can still slightly adapt them if it's really unavoidable but otherwise just apply them more and more.

Co-authored-by: Matthias Grob <[email protected]>
en/contribute/code.md Outdated Show resolved Hide resolved
en/contribute/code.md Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator

Nice. So we're no longer defining our coding standard by a tool (astyle)? If there is a toolchain that manages this it would be good to at least mention it. And if the intent is that clang will do it one day then putting that in a note with a link to the issue PR in which that is addressed would be even better - because it allows easier later cleanup.

Do we need to say something specific about a dislike for magic numbers?

@dagar LGTM - if you're happy just merge.

@tstastny
Copy link
Author

tstastny commented Jul 1, 2022

Do we need to say something specific about a dislike for magic numbers?

Added :)

So we're no longer defining our coding standard by a tool (astyle)? If there is a toolchain that manages this it would be good to at least mention it. And if the intent is that clang will do it one day then putting that in a note with a link to the issue PR in which that is addressed would be even better - because it allows easier later cleanup.

Yes clang format is intended to take this over (and eventually clang tidy doing even more heavy lifting) - I added a note/link in the PR description. @MaEtUgR if you think we could already add something about this to this document - please feel free to commit directly to this PR.

NOTE: clang format will be used where astyle was before, we will need to remember to update this doc with instructions for e.g. the hook setup and auto formatting make command (does it stay the same? make format ?) @MaEtUgR @hamishwillee
^see PX4/PX4-Autopilot#19855

@tstastny tstastny force-pushed the update-style-guide branch from 289e4ca to 33fa25a Compare July 1, 2022 08:47
@hamishwillee
Copy link
Collaborator

hamishwillee commented Jul 6, 2022

Thanks @tstastny

This looks good to me. I'm going to merge and we can iterate.

I added you and @MaEtUgR as owners for adding clang update when that is ready: #1927

@hamishwillee hamishwillee merged commit c83a329 into main Jul 6, 2022
@hamishwillee hamishwillee deleted the update-style-guide branch July 6, 2022 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants