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

Add linting/codestyle control #12

Closed
glitchcore opened this issue Aug 14, 2020 · 12 comments · Fixed by #108
Closed

Add linting/codestyle control #12

glitchcore opened this issue Aug 14, 2020 · 12 comments · Fixed by #108
Labels
Feature Request New feature or user-story you wanna add to flipper

Comments

@glitchcore
Copy link
Contributor

Nobody want to work with dirty code and I suggest:

  • Automatic formatting code tool that allow check C/C++ and Rust code style or make formatting by one command
  • CI pipeline to prevent merge of pull requests if the style check was not successful

Also need to add information about code style checking to contribution guide.

I hear about clang format

@glitchcore glitchcore added Feature Request New feature or user-story you wanna add to flipper discussion labels Aug 14, 2020
@glitchcore glitchcore pinned this issue Aug 17, 2020
@glitchcore glitchcore unpinned this issue Aug 21, 2020
@glitchcore
Copy link
Contributor Author

(from #39) At this time I prefer default rustfmt settings as code style (https://rust-lang.github.io/rustfmt/?version=master&search=) even in C/C++ code (where applicable, of course)

I suggest adapt rustfmt defaults to clang format

@glitchcore
Copy link
Contributor Author

glitchcore commented Aug 22, 2020

@officialdarksheao can I assign you to this task?

@glitchcore
Copy link
Contributor Author

glitchcore commented Aug 22, 2020

We have a lot of thirdparty SDK code in target_f* folders now, I don't like its style and maybe we should ignore it

@glitchcore
Copy link
Contributor Author

@nikita-b can I assign you to this task?

@nikita-b
Copy link
Contributor

@glitchcore Yes, sure

@glitchcore
Copy link
Contributor Author

@nikita-b Do you still want to do this task?

@pavel-demin
Copy link

pavel-demin commented Sep 7, 2020

CI pipeline to prevent merge of pull requests if the style check was not successful

Here's an example of an action that executes clang-format on each push. If the code changes after formatting, this action creates an additional commit with a fixed commit message ("apply clang-format").

What do you think about this approach?

@nikita-b
Copy link
Contributor

nikita-b commented Sep 8, 2020

@glitchcore Yes, sorry for the delay. I'll start today.

@reendael
Copy link

reendael commented Sep 8, 2020

CI pipeline to prevent merge of pull requests if the style check was not successful

Here's an example of an action that executes clang-format on each push. If the code changes after formatting, this action creates an additional commit with a fixed commit message ("apply clang-format").

What do you think about this approach?

Wouldn't it be better if a pre-commit hook ran clang-format check, detected if the output differed from the input file and notified the developer with a warning and a hint to run clang-format manually?

This way developers are encouraged to periodically (or automatically) run clang-format themselves and even more importantly, will prevent pollution of git history with formatting commits.

@pavel-demin
Copy link

Wouldn't it be better if a pre-commit hook ran clang-format check, detected if the output differed from the input file and notified the developer with a warning and a hint to run clang-format manually?

Since the pre-commit hooks run locally on the developer's machine, and the clang-format parameters change from version to version, we'll need to make sure that all developers have the same clang-format version.

@pavel-demin
Copy link

pavel-demin commented Sep 8, 2020

Formatting errors can be detected with the following command:

clang-format --dry-run --Werror --ferror-limit=1 <file>

@nikita-b
Copy link
Contributor

nikita-b commented Sep 8, 2020

CI pipeline to prevent merge of pull requests if the style check was not successful

Here's an example of an action that executes clang-format on each push. If the code changes after formatting, this action creates an additional commit with a fixed commit message ("apply clang-format").
What do you think about this approach?

Wouldn't it be better if a pre-commit hook ran clang-format check, detected if the output differed from the input file and notified the developer with a warning and a hint to run clang-format manually?

This way developers are encouraged to periodically (or automatically) run clang-format themselves and even more importantly, will prevent pollution of git history with formatting commits.

Pre-commit hook is a great idea, but I think it'll be more complicated for contributors with the Windows machine and new contributors. I suggest starting with CI and git history won't be a problem if we squash commits before a merge.

redlink2 referenced this issue in redlink2-archive/flipperzero-redlinked May 18, 2022
thzinc pushed a commit to thzinc/flipperzero-firmware that referenced this issue Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request New feature or user-story you wanna add to flipper
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants