Add support for ignoring files, such as package-lock.json

[dowrow]

I'm really interested in this. What would be the best way to implement it? I've been checking the github::calculate_total_modifications() method and the Github API docs and the only approach I found would be:

• Allow a 8th argument to be passed to the action (ignore_files), which would be a string with the comma-separated files to ignore.
• Inside calculate_total_modifications() define a variable totalLines = deteledLines + addedLines (current returned value)
• Loop over every file in the PR (using this endpoint GET /repos/:owner/:repo/pulls/:pull_number/files) and check if the filename matches one of the ignored_files. If it does, substract its (deletedLines + addedLines) from the totalLines.

Does this make sense? Am I missing anything?

[JavierCane]

It would be awesome to support this 🙌

@dowrow, your approach seems to completely make sense 🙂

The only thing I would suggest would be using a YAML sequence instead of a comma separated string for specifying the different files to ignore. That is, being able to specify it as files_to_ignore: ['package-lock.json', 'composer.lock'] instead of files_to_ignore: 'package-lock.json, composer.lock'.

Example of how the README.md complete example would look like:

name: labeler

on: [pull_request]

jobs:
  labeler:
    runs-on: ubuntu-latest
    name: Label the PR size
    steps:
      - uses: codelytv/pr-size-labeler@v1
        with:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
          xs_max_size: '10'
          s_max_size: '100'
          m_max_size: '500'
          l_max_size: '1000'
          fail_if_xl: 'false'
          message_if_xl: 'This PR is so big! Please, split it 😊'
          files_to_ignore: ['package-lock.json', 'composer.lock']

What do you think?

[ddalpange]

Thanks this project.

is there anything working on this issue?

If nobody solving it, I will contribute to solve this issue .

@JavierCane @rgomezcasas

[dowrow]

Hey @ddalpange, that would be great! I can't really move forward with this issue at the moment.

[mrchief]

@rgomezcasas @dowrow what about this commit? Seems like it does what is being described here: https://github.com/martincleto/pr-size-labeler/commit/7fc2823c1b87cc499e7ee2862edc781f50f1d89d

[mharris717]

@mrchief does it? It looks like it adds the argument, but doesn't actually use it

[mrchief]

@mharris717 you're right - seems like it goes all the way except ignoring actual changes.

[magnattic]

Wouldn't the correct solution be to mark those files in the .gitattributes file so they count towards the lines changed in Github?

I haven't tested this yet myself but that seems to be exactly what the linguist-generated flag is for: https://github.com/github/linguist/blob/master/docs/overrides.md#generated-code

So for the example given just add a .gitattributes file with

package-lock.json linguist-generated=true

to your repo and hopefully those line changes are then ignored in the labeller as well?

[favmd]

Dear @kevindice @dowrow @JavierCane et al.

I've been using the pr-size-labeler workflow for quite a while now (it's simple yet super helpful!), so I gave this issue a try: Have a look at this one commit from my forked repo: https://github.com/favmd/pr-size-labeler/commit/992a73aa3e13a366211a8e4180fd9f0886e917b4

The code basically does what has been described above:

• Allow a whitespace(!)-separated list of files to be ignored (files_to_ignore)
• If files_to_ignore is empty, the old code will be used (see below for the rationale behind this!)
• If files_to_ignore is given, it will loop through the list of files and count the changes

Plus: Minor modifications:

• Add missing message_if_xl parameter description to README.md
• Fix issue #29

Note however that my code does have its LIMITATIONS and should be tested by someone else too:

• Filenames with whitespaces are not supported (never saw that in auto-generated filenames anyway...)
• files_to_ignore is global (as described above), i.e., filenames will be ignored in every part of the repo
• Only the first 100 files are taken into account ❗ (note that the Pulls API endpoint GET /repos/{owner}/{repo}/pulls/{pull_number}/files is paginated and limited to 3000 files max (100 files max per page))

I can live with that limitation due to the fact that we don't open PRs with hundreds of changed files, but this is the reason why my code will fall back to the old code if files_to_ignore is empty: to ensure that someone not using the new parameter does not have this limitation at all and falls back to the API endpoint without /files at the end.

P.S. Dear @magnattic: Your approach did not work for me, but maybe I just missed some little detail.

[magnattic]

@favmd You are right, after testing this myself I think the approach does not work as expected, unfortunately. The lines are still counted by github even if the file is marked as linguist-generated=true.

Maybe that's also an issue we could raise with github, I feel like those should be excluded from the diff altogether.

EDIT: ℹ️ I opened an issue with Github, please support it by upvoting! https://github.com/github/feedback/discussions/12343

[magnattic]

Regarding this feature request: It would be awesome if files_to_ignore would support glob patterns, so the syntax would be in line with e.g. .gitignore.

[favmd]

Dear @magnattic Upvoted your discussion! Let's see where this leads.

You are right: From the docs I would have expected your approach to work just fine.

Regarding this feature request: It would be awesome if files_to_ignore would support glob patterns, so the syntax would be in line with e.g. .gitignore.

That's way more than I need, but a wonderful idea (maybe for another Issue/PR if this one gets approved?).

Feel free to fork and improve my approach :)

[szamanr]

@favmd nice, thanks for working on that. this seems ready for review, could you open a pull request? not sure if the repo maintainers will see it otherwise.

[favmd]

@szamanr I opened PR #33 👍

[rgomezcasas]

Feature implemented thanks to @favmd 🎉

You can see how to use it in the documentation.

Thanks!