espressif / arduino-esp32

Arduino core for the ESP32
GNU Lesser General Public License v2.1
13.57k stars 7.4k forks source link

Add action to auto format pull requests and pushes #7975

Closed FernandoGarcia closed 6 months ago

FernandoGarcia commented 1 year ago

Related area

Software contribuition

Hardware specification

All boards

Is your feature request related to a problem?

Hi!

I'm have noticed some maintainers and reviewers complaining about code formatting in PR here.

I would like to request to someone with more skills than me to create a script to apply formatting to all contributions for this repository.

I know that Espressif team is using Artistic style as formatter.

I was doing some tests with this action but later I have noticed that it only does a dry run for check for non compliant code.

Anyway I think the script used for this action can be useful as base to create the action for this repository.

I know there's security problems while triggering an action from public fork but I'm sure that this problem can be solved.

Alternatively I would like to suggest the use of lint with clang formatter as shown on example below.

On example below the commit with formatting changes is always signed by someone with write permission in this repository.

Best regards.

Describe the solution you'd like

Here an basic example:

name: Run clang-format Linter

on: [push]

jobs:
  build:
    runs-on: ubuntu-latest

    steps:
    - uses: actions/checkout@v3.4.0
    - uses: DoozyX/clang-format-lint-action@v0.15
      with:
        source: '.'
        extensions: 'h,cpp,c,ino'
        clangFormatVersion: 14
        inplace: True
    - uses: EndBug/add-and-commit@v4
      with:
        author_name: someManteiner
        author_email: somemanteiner@example.com
        message: 'Committing clang-format changes'
      env:
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Describe alternatives you've considered

No response

Additional context

No response

I have checked existing list of Feature requests and the Contribution Guide

igrr commented 1 year ago

For reference, in most new Espressif projects we use pre-commit framework to format the code at commit time. For example, https://github.com/espressif/esp-bsp/blob/160c0d66d851c3e247de4c535dc8ddad5a7e585f/.pre-commit-config.yaml#L6 This option could be considered in Arduino-esp32 as well.

mrengineer7777 commented 1 year ago

I love the idea of a consistent coding style.

VojtechBartoska commented 1 year ago

Sharing this as another option: https://peterevans.dev/posts/github-actions-how-to-automate-code-formatting-in-pull-requests/

In general, we would like to implement this, we are now in phase of investigating the best option how to do this.

mrengineer7777 commented 1 year ago

Sharing this as another option: https://peterevans.dev/posts/github-actions-how-to-automate-code-formatting-in-pull-requests/

In general, we would like to implement this, we are now in phase of investigating the best option how to do this.

"Important caveat 1: Due to token restrictions on public repository forks these workflows do not work for pull requests raised from forks. Private repositories can be configured to enable workflows from forks to run without restriction." Does this mean user submitted PRs wouldn't format? Testing needed!

FernandoGarcia commented 1 year ago

Yes, formatting is not applied to PR from forks. This question is explained on link in my first post. To solve it the maintainers should stop complaining about formatting on PR because the formatting will be applied when the PR is merged.

VojtechBartoska commented 1 year ago

yea, tokens restriction is somehow manageable but there are other cons, sharing 2 points from @igrr:

  1. It creates an inconsistency between the developer's local branch and the branch on Github. If the developer wants to push more changes, they first need to merge the changes done by autopep8
  2. It adds pointless code formatting commits to the commit history, instead of integrating the changes into the original commits which have modified the code.

So far pre commit hook looks as a best option, there are some requirements on developers side and also we need to think about some special cases as option to skip it, it's influence on review process, exceptions and so on.