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

feat(ci): auto-fix formatting and linting on new PRs #108

Open
maxpatiiuk opened this issue Jan 15, 2024 · 1 comment
Open

feat(ci): auto-fix formatting and linting on new PRs #108

maxpatiiuk opened this issue Jan 15, 2024 · 1 comment
Labels
internal-improvements Related to internal publishing, building, workflows, formatting & linting needs-investigation Further information is requested

Comments

@maxpatiiuk
Copy link

maxpatiiuk commented Jan 15, 2024

This is in response to your Cool things in '23 post:

I also really want a GitHub action/bot to apply clippy --fix and rustfmt format on the current PR. I don't know how I feel asking the author to do the sort of bookkeeping in a project if simple checks fail. It would be nice if I could comment @my-bot try fix lints and formatting and it runs it and commits the fixes (rather than me having to checkout and do it manually). Is this even a good idea, does it change who authored it when it appears in git blame`? If you know a simple solution feel free to PR it.

I do not know know about rust-tooling, but in JavaScript-land there exists lint-staged to run pre-configured linters on files that were changed between main and the head of a branch the PR is on.

There is also husky for running lint-staged as a pre-commit hook in Git (so that devs get feedback even before commit is made, even if they don't have rustfmt in their IDEs)

I am not opening a PR for this as I am not sure if you would be open to having npm-based tools added in this repository, but here is how the above could be configured:

Create lint-staged.config.js:

const autoFix = !process.env.NO_AUTO_FIX;

module.exports = {
  "*.{rs}": [`clippy ${autoFix ? "--fix" : ""}`, 'rustfmt format'],
};

Install dependencies:

npm install --save-dev lint-staged husky

Add this to scripts section in package.json:

    "lint": "lint-staged --config lint-staged.config.js",

Create GitHub Action workflow lint.yml:

name: Lint code

on:
  push:
  pull_request:

jobs:
  linting:
    if: github.event_name != 'pull_request' || github.event.pull_request.draft == false
    steps:

      - uses: actions/checkout@v3
        with:
          fetch-depth: 0
          # Fetch a branch, not detached HEAD (to allow committing changes)
          ref: ${{ github.event_name == 'pull_request' && github.head_ref || github.ref_name }}

      - uses: actions/setup-node@v3

      - name: Install dependencies
        run: npm ci

      - name: Lint packages
        run: |
          if [ ${{ github.event_name }} == 'pull_request' ]; then
            npx lint --diff="origin/${{ github.base_ref }}...origin/${{ github.head_ref }}"
          else
            npx lint --diff "HEAD~1...HEAD"
          fi

      - name: Commit linted files (if made any changes)
        if: github.event_name == 'pull_request'
        # Creates a new commit. Amending an existing commit is a bad idea
        # because:
        # - Would require original committer to force pull. May cause merge
        #   conflicts
        # - Changes that require linting might have been made over several
        #   commits. If you amend the last commit, than the fixes for all
        #   commits would be in the last commit.
        run: |
          # Check if any files were changed by autofix
          git add .
          if git diff-index --quiet HEAD --; then
            echo "Linters did not detect any issues. Good job!"
          else
            # Set committer to the person who committed the last commit on this branch:
            git config --local committer.name "$( git log -1 --pretty=format:'%cn' )"
            git config --local committer.email "$( git log -1 --pretty=format:'%ce' )"
            # Author commit as a GitHub Action. Commits authored by GitHub
            # Action do not trigger GitHub Action workflows. This avoids a
            # cycle of Action triggering itself in a loop.
            git config --global author.email "github-actions"
            git config --global author.name "41898282+github-actions[bot]@users.noreply.github.com"

            git commit \
              --message "chore(lint): lint code with clippy and rustfmt" \
              --message "Triggered by ${{ github.event.pull_request.head.sha }} on branch ${{ github.head_ref }}" \
              --no-verify
            git push --no-verify
          fi

Alternatively, instead of using npm's lint-staged, there is a GitHub action dorny/paths-filter
Example usage in https://github.com/specify/specify7/blob/production/.github/workflows/test.yml

@kaleidawave
Copy link
Owner

Yay someone read my post!

Interesting that lint-staged can work on Git diffs. Unfortunately it seems that in Rust land, clippy lints the whole project (probably because it needs global information).

The code under name: Commit linted files (if made any changes) looks useful for applying the changes. This is cool that it keeps the author

git config --local committer.name "$( git log -1 --pretty=format:'%cn' )"
git config --local committer.email "$( git log -1 --pretty=format:'%ce' )"

I wonder if it is only possible to trigger only on a comment. Or maybe just doing it automatically might be fine...? Will think about it 👍

@kaleidawave kaleidawave added needs-investigation Further information is requested internal-improvements Related to internal publishing, building, workflows, formatting & linting labels Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-improvements Related to internal publishing, building, workflows, formatting & linting needs-investigation Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants