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 Checker and Pre-commit hook CI #9132

Merged
merged 26 commits into from
Apr 15, 2024

Conversation

lucasssvaz
Copy link
Collaborator

@lucasssvaz lucasssvaz commented Jan 17, 2024

Description of Change

This PR aims to add pre-commit hooks into the CI by using pre-commit.ci lite. It adds all necessary files to make clang-format, black, flake8, prettier and codespell follow the default Arduino styling. This PR also adds a .editorconfig file to help with default Arduino indentation for different file types and vale with the default Espressif settings.

The default Arduino styles were taken from: https://github.com/arduino/tooling-project-assets

This PR requires adding the pre-commit.ci lite bot to the repository and is easily extendable for future hooks (like Markdown linting)

As decided in the meeting this should only be merged after refactoring the code base (possibly after all open PRs have been resolved).

Tested locally and on my fork (lucasssvaz#2).

Description of each Hook

clang-format (C/C++ Formatter)

This hook will format the C, C++ and sketch files according to the Arduino default styling (LLVM style based). Shouldn't cause any problem as it will only reformat the code to follow the specs.

black (Python Formatter)

Black will format Python files to it's default configuration. The only changes Arduino does to the default config is line-length=120. This will also fix most errors pointed out by flake8. Shouldn't cause any problem by itself as it is only a formatter. Note that it doesn't change long comment lines automatically.

flake8 (Python Linter)

The aim of this hook is to enforce Python styling and good practices. It uses error codes generated by itself and pycodestyle for pointing out what should be changed. Arduino ignores error W503 and sets the maximum complexity score to 10.

Note that black can't fix all errors pointed by flake8. bare expect and too complex are such examples. This should cause the need for some manual edits depending on how the code was written.

prettier (YAML Formatter)

This hook uses prettier to format only YAML files. It doesn't have configuration options as Arduino uses the default options for formatting. Won't cause any problems by itself.

codespell (Spell Checker)

Checks for common spelling mistakes and automatically converts EN_GB to EN_US. It is configured to be run manually as it sometimes can detect false positives (During my local testing, out of ~200 spelling mistakes detected, 4 were false positives). For example swapping a variable finalY to finally. This won't be run by the CI and can be run locally using pre-commit run codespell --hook-stage manual.

vale (Prose Linter)

Checks for mistakes and improvements in natural language (english). Used in documentation files with the default Espressif configuration. Can't auto fix files.

Related links

Closes #7975.

Copy link
Contributor

github-actions bot commented Jan 17, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Add Cache and remove pre-commit action":
    • summary looks empty
    • type/action looks empty
  • the commit message "Add Config":
    • summary looks empty
    • type/action looks empty
  • the commit message "Add filter":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix flake and make Vale manual":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix":
    • summary looks empty
    • type/action looks empty
  • the commit message "Improve bot message":
    • summary looks empty
    • type/action looks empty
  • the commit message "Improve cache tag":
    • summary looks empty
    • type/action looks empty
  • the commit message "Improve caching":
    • summary looks empty
    • type/action looks empty
  • the commit message "Remove freeze":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update action":
    • summary looks empty
    • type/action looks empty
  • the commit message "Use latest stable Python 3 version":
    • summary looks empty
    • type/action looks empty
  • the commit message "[pre-commit.ci lite] apply automatic fixes":
    • summary looks empty
    • type/action looks empty

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

⚠️ Please consider squashing your 26 commits (simplifying branch history).

👋 Hello lucasssvaz, we appreciate your contribution to this project!


Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against bffb8dc

@lucasssvaz lucasssvaz changed the title CI test Style Checker CI and Pre-commit hook Jan 17, 2024
@lucasssvaz lucasssvaz force-pushed the ci/style_check branch 7 times, most recently from 64b00c3 to c69ca14 Compare January 22, 2024 13:23
@CLAassistant
Copy link

CLAassistant commented Jan 22, 2024

CLA assistant check
All committers have signed the CLA.

@lucasssvaz lucasssvaz force-pushed the ci/style_check branch 3 times, most recently from 958bba1 to c69ca14 Compare January 22, 2024 15:10
@VojtechBartoska VojtechBartoska added the Status: In Progress Issue is in progress label Jan 24, 2024
@lucasssvaz lucasssvaz changed the title Style Checker CI and Pre-commit hook Style Checker and Pre-commit hook CI Jan 24, 2024
@lucasssvaz lucasssvaz self-assigned this Jan 24, 2024
@espressif espressif deleted a comment from VojtechBartoska Jan 24, 2024
@lucasssvaz
Copy link
Collaborator Author

@VojtechBartoska We'll also need to add the bot to the list of signed users of CLAassistant as described in here:

cla-assistant/cla-assistant#173 (comment)

@lucasssvaz lucasssvaz marked this pull request as ready for review January 24, 2024 20:49
@lucasssvaz lucasssvaz added Status: Review needed Issue or PR is awaiting review and removed Status: In Progress Issue is in progress labels Jan 24, 2024
@lucasssvaz
Copy link
Collaborator Author

lucasssvaz commented Jan 25, 2024

Arduino also enforces markdown formatting and linting for their documentation. It wouldn't make much sense to follow Arduino standards for our documentation but we can add our own formatting and linting rules.

@pedrominatel Do we have any specific rule set that we should follow (or that you would like us to follow) ?

@lucasssvaz lucasssvaz force-pushed the ci/style_check branch 15 times, most recently from a3bb162 to 7bb7270 Compare April 15, 2024 12:16
@lucasssvaz lucasssvaz added Status: Pending Merge Pull Request is ready to be merged and removed Status: Blocked upstream 🛑 PR is waiting on upstream changes to be merged first Status: Pending Merge Pull Request is ready to be merged labels Apr 15, 2024
@lucasssvaz lucasssvaz added Status: Pending Merge Pull Request is ready to be merged and removed Status: Pending Merge Pull Request is ready to be merged labels Apr 15, 2024
@me-no-dev me-no-dev merged commit 4909dec into espressif:master Apr 15, 2024
40 checks passed
me-no-dev pushed a commit that referenced this pull request Apr 15, 2024
* Add Config

* Add Cache and remove pre-commit action

* [pre-commit.ci lite] apply automatic fixes

* Remove freeze

* Fix

* Update action

* Use latest stable Python 3 version

* Improve caching

* Improve cache tag

* Improve bot message

* Fix flake and make Vale manual

* Add filter

---------

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Co-authored-by: Rodrigo Garcia <[email protected]>
@lucasssvaz lucasssvaz deleted the ci/style_check branch April 15, 2024 19:51
P-R-O-C-H-Y pushed a commit to P-R-O-C-H-Y/arduino-esp32 that referenced this pull request Apr 16, 2024
* Add Config

* Add Cache and remove pre-commit action

* [pre-commit.ci lite] apply automatic fixes

* Remove freeze

* Fix

* Update action

* Use latest stable Python 3 version

* Improve caching

* Improve cache tag

* Improve bot message

* Fix flake and make Vale manual

* Add filter

---------

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Co-authored-by: Rodrigo Garcia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Add action to auto format pull requests and pushes
8 participants