Skip to content

Add action to auto format pull requests and pushes #7975

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

Closed
1 task done
FernandoGarcia opened this issue Mar 18, 2023 · 6 comments · Fixed by #9132
Closed
1 task done

Add action to auto format pull requests and pushes #7975

FernandoGarcia opened this issue Mar 18, 2023 · 6 comments · Fixed by #9132
Assignees
Labels
Status: Blocked upstream 🛑 PR is waiting on upstream changes to be merged first Type: CI & Testing Type: Feature request Feature request for Arduino ESP32
Milestone

Comments

@FernandoGarcia
Copy link

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/[email protected]
    - uses: DoozyX/[email protected]
      with:
        source: '.'
        extensions: 'h,cpp,c,ino'
        clangFormatVersion: 14
        inplace: True
    - uses: EndBug/add-and-commit@v4
      with:
        author_name: someManteiner
        author_email: [email protected]
        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

  • I confirm I have checked existing list of Feature requests and Contribution Guide.
@FernandoGarcia FernandoGarcia added the Type: Feature request Feature request for Arduino ESP32 label Mar 18, 2023
@igrr
Copy link
Member

igrr commented Mar 19, 2023

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
Copy link
Collaborator

I love the idea of a consistent coding style.

@VojtechBartoska
Copy link
Contributor

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.

@VojtechBartoska VojtechBartoska moved this from Todo to Under investigation in Arduino ESP32 Core Project Roadmap May 17, 2023
@VojtechBartoska VojtechBartoska added the Status: In Progress ⚠️ Issue is in progress label May 17, 2023
@mrengineer7777
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Contributor

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.

@VojtechBartoska VojtechBartoska modified the milestones: 3.0.0, 3.1.0 Jan 17, 2024
@VojtechBartoska VojtechBartoska added Status: Needs investigation We need to do some research before taking next steps on this issue Status: In Progress ⚠️ Issue is in progress and removed Status: In Progress ⚠️ Issue is in progress Status: Needs investigation We need to do some research before taking next steps on this issue labels Jan 17, 2024
@VojtechBartoska VojtechBartoska moved this from Under investigation to In Progress in Arduino ESP32 Core Project Roadmap Jan 24, 2024
@lucasssvaz lucasssvaz added Status: Review needed Issue or PR is awaiting review and removed Status: In Progress ⚠️ Issue is in progress labels Jan 26, 2024
@VojtechBartoska VojtechBartoska moved this from In Progress to In Review in Arduino ESP32 Core Project Roadmap Feb 21, 2024
@lucasssvaz lucasssvaz added Status: Blocked upstream 🛑 PR is waiting on upstream changes to be merged first and removed Status: Review needed Issue or PR is awaiting review labels Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Blocked upstream 🛑 PR is waiting on upstream changes to be merged first Type: CI & Testing Type: Feature request Feature request for Arduino ESP32
Projects
Development

Successfully merging a pull request may close this issue.

6 participants