Only lint modified files

[theoomoregbee]

I was going through the codebase to see how feasible to add this feature because we currently need it.

Suggestion

I was thinking of adding an extra param [linter]_only_changed_files: boolean (we can decide on the best name to go with).

Then checking doing a quick diff of the current branch with

FILES = git diff --diff-filter=$filter --name-only $baseBranch | grep -E $extensions

where:

• $filter: can be defaulted to ACM (added, copied and modified), more: https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---diff-filterACDMRTUXB82308203
• $baseBranch: we can get the base branch from event.pull_request.base.ref
• $extensions: e.g .js|.ts or we use the default extension from the lint configuration

I see you already swap branch for hasFork here: https://github.com/samuelmeuli/lint-action/blob/a820202c34156c11c68a7c534178ac66e12758b9/src/git.js#L22

We just need to do the same thing with the base repository:

• check if it's a fork, if so fetch it and add it to maybe base-fork remote locally
• else if it's not a fork, we don't need to fetch
• with the above steps, we can easily infer the base remote and branch name

Don't know if this is what you want to support @samuelmeuli else I can spin up a PR for this.

@samuelmeuli wdyt?

[samuelmeuli]

It would be great to have this working. I'm just not quite sure if this will work nicely with GitHub annotations (see #1).

In any case, this would be a follow-up to #46. It might also make sense to delegate the identification of modified files to another action (e.g. file-changes-action) and use that action's output as input for the [linter]_files option.

[ozum]

This should be optional, because if a lint rule is changed, it should lint all source files again for new rules whether those files changed or not.

[theoomoregbee]

@samuelmeuli went through #1 will read on github annotation over the weekend and see if there's a possibility for getting this in.

In any case, this would be a follow-up to #46. It might also make sense to delegate the identification of modified files to another action (e.g. file-changes-action) and use that action's output as input for the [linter]_files option.

Nice fallback

[panda0603]

@theoomoregbee @samuelmeuli any progress on this issue? Thanks

[theoomoregbee]

@theoomoregbee @samuelmeuli any progress on this issue? Thanks

We had a blocker kinda as mentioned in #1 but had not gotten free time to go over GitHub annotations yet to see if there's a way around it.

Will add it to my todo list for the week now and see if I can go over it on or before the weekend.

[theoomoregbee]

Did some research regarding #1 and found a GitHub action that helps with annotation via a .json file: https://github.com/Attest/annotations-action.

They just released a new version of runner that helps with composite actions https://docs.github.com/en/actions/creating-actions/about-actions#composite-run-steps-actions

But it's not fully ready to support calling an action within another action yet (which I think is the main reason we need the composite action though lol actions/runner#646 (comment)

Here are the next steps for composite actions: actions/runner#646

When expected action within action is released, we can leverage

• file changes(conditional step, if to check for modified files or entire branch) https://github.com/trilom/file-changes-action
• Run eslint and parse errors/warnings messages to follow https://github.com/Attest/annotations-action/blob/master/annotations.json (type definition: https://github.com/Attest/annotations-action/blob/master/src/annotation.ts)

[github-actions]

A stale label has been added to this issue because it has been open 15 days with no activity. To keep this issue open, add a comment within 5 days.

[shahyar]

I think the only issue is that the linting path is hard-coded to ".". If that were a parameter, then this whole task could be avoided. It would be up to the individual actions to determine how to fill that value.

I originally tried using [linter]_command_prefix, but because it's used in verifySetup, it isn't useful here. So, I put together a proof-of-concept of how this can be done. #533

1. On a PR, you need the target branch (github.base_ref), and on push you need the last commit before the push commits were added (github.event.push.before). On other types of events, I don't know.
2. The static lint method needs to stop pre-determining what files to use. In eslint, it uses ".". This is now a param.
3. The static lint method also needs to know how to prefix just the command, so we can tell it to use git diff ... | xargs.
4. We need to know what files were changed, and that's done with git diff -z --relative --diff-filter=ACMR --name-only $BASEHASH -- ....

Lastly, my example does not take into account changes to rules. If any of those files change, you'll want to trigger a complete lint rather than just changed files. This could be accomplished with dorny/paths-filter@v2, and using it to conditionally set only_changes_from.

Here's a sample config, linting a subdirectory:

    steps:
      # We need to check out the repo history... perhaps fetch-depth: 0 is not needed? TBD
      - name: Checkout repository
        uses: actions/checkout@v3
        with:
          fetch-depth: 0
          ref: ${{ github.event.pull_request.head.sha || github.sha }}

      # Also need to test if this is still necessary
      - run: |
          git fetch --no-tags --depth=1 origin ${{ github.event.pull_request.base.sha || github.event.push.before }}

      - name: Setup Node.js
        uses: actions/setup-node@v3
        with:
          node-version: 16
          cache: 'npm'
          cache-dependency-path: 'packages/foobar/package-lock.json'

      # Need to install eslint dependencies in the subdirectory (note in this example root/package.json must have eslint)
      - name: Install Node.js dependencies
        run: |
          npm ci
          cd packages/foobar && npm ci

      # Now actually lint by setting files to an empty string, and telling it to use the base hash to compare to
      - name: Lint packages/foobar
        uses: shahyar/lint-action@master
        with:
          only_changes_from: ${{ github.event.pull_request.base.sha || github.event.push.before }}
          eslint: true
          eslint_dir: packages/foobar/
          eslint_files: ""

[messers2]

Any news on this?

[shahyar]

This package is basically unmaintained. The only changes getting merged are new linters. You can see my draft PR above (forked at shahyar/lint-action@master) which adds the desired functionality to eslint and prettier.

[wesleyschlenker]

It's incredibly frustrating that annotations don't work, but thank you @shahyar for the fork you've made. I'm really hopeful that someone smarter than me can figure out a way to work around the check_run API limitations pointed out here.

Edit: it looks like maybe they figured it out over here https://github.com/tj-actions/eslint-changed-files

[lkraav]

Edit: it looks like maybe they figured it out over here https://github.com/tj-actions/eslint-changed-files

It's what we're using for eslint, but as stylelint was next, requiring similar mechanics, I started looking around for combo-linters, and ended up here. One thing's for sure: "only lint modified files" is a hard requirement.

[mattcasey]

I wandered into this thread trying to research how to do the same. Just FYI wanted to share that the changed file logic lives in a separate action here: https://github.com/tj-actions/changed-files/