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

DevEnv: Finish out the astylerc settings #19009

Open
TunaLobster opened this issue Oct 21, 2021 · 8 comments
Open

DevEnv: Finish out the astylerc settings #19009

TunaLobster opened this issue Oct 21, 2021 · 8 comments

Comments

@TunaLobster
Copy link
Contributor

Feature request

Problem:
Currently the astylerc file works well for some files, but it does not work well for headers. It also does not include all of our formatting guidelines causing some odd results when using it and not following all of the guidelines at the start.

Solution:
Going through the long list of defaults that astyle implements and correcting the ones that need to be changed. Adding in checking for header files.

Platform
[x] All
[ ] AntennaTracker
[ ] Copter
[ ] Plane
[ ] Rover
[ ] Submarine

@khancyr
Copy link
Contributor

khancyr commented Oct 21, 2021

the issue with the formatter is that we aren't conform to our own style on some files for readability so, I doubt it will work.
But it could be nice to have a little update on it.
Another linter to consider would be clang-format as it is well used on IDE.

@Rohan-Dah
Copy link

Hi there folks... is this issue still open? I would love to work on it but I'll need a bit of guidance as it will be my first issue..

@TunaLobster
Copy link
Contributor Author

I think I might have done that here. This isn't merged so nothing has been completed really. TunaLobster/ardupilot@pr/udpate-pre-commit...TunaLobster:ardupilot:pre/add-astyle-pre-commit

There might be a few more optional astyle things that I missed while reading through the docs. Like khancyr said, if there is a better linter for us, that would work too.

@Rohan-Dah
Copy link

So I went through the style documentation page and understood what astyle is and how it is being used in the directory. This is the link of what I referred https://ardupilot.org/dev/docs/style-guide.html

Here I found below things that can be fixed here -> ardupilot/Tools/CodeStyle/astylerc

  1. 'Trailing whitespaces' that needs fixing.
    ->As referred above, a linter can be used here to fix this. Clang tidy can be used here serving as a linter.

  2. Line Breaks:
    Single statements (Each statement should get its own line)
    -> This can be done by setting 'keep-one-line-statements' as true.

  3. Braces:
    Function definitions: place each brace on its own line. For methods inside a header file, braces can be inline.
    -> A linter can play its role here.

Control statements (if, while, do, else) should always use braces around the statements
-> It can be done by setting value of 'add-brackets' as true

Other Braces:
-> Fix for this too can be linter

  1. For 'Names', 'Functions and variables' and rest of the page there aren't specific options in astyle that can be used however it could be manually named as required.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Nov 22, 2023

I found a major problem with the astyle setting.
#25579 (comment)

We're using astyle in parts of the codebase and enforcing it in CI. But, it's doing nonsensical things.

Did anyone have a plan for why the astyle config is added to the repo and used, but it's not apparently finished?

@khancyr
Copy link
Contributor

khancyr commented Nov 22, 2023

astyle was there years ago, nobody enforced it nor make a clean configuration. We can switch to another tool if we can have some consitent. Main issue is that we cannot make a first pass to clean the full repo as that breaks all PRs and history. Second one, is that on some part of the code we are doing manual indention for "easier reading" and linter cannot cope with those

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Nov 23, 2023

Ok. If astyle is known to be broken, perhaps we need to remove it from the repo and remove all tooling using it.

clang-tidy supports adding ignores, so you can do manual indent and tell the linter not to touch it. Or, configure the linter to manually indent your matrices.

See this PR for a partial solution to the history problem: #25609

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Nov 23, 2023

For the lack of correct formatting in a struct with brace initialization, upgrading to astyle 3.2 should fix it. Ubuntu 22 comes with 3.1.
https://astyle.sourceforge.net/notes.html
fixed struct bitfield indentation (#534)
https://sourceforge.net/p/astyle/bugs/534/

Ryanf55 added a commit to Ryanf55/ardupilot that referenced this issue Nov 23, 2023
* See ArduPilot#19009 (comment)

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants