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

feature: Add support for ignoring files (#9) #33

Merged
merged 1 commit into from
Mar 20, 2022

Conversation

favmd
Copy link
Contributor

@favmd favmd commented Mar 17, 2022

The code basically does what has been described / discussed in issue #9:

  • 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:

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, i.e., filenames will be ignored in every part of the repository
  • 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.

@rgomezcasas
Copy link
Member

Amazing! I'll try it today.

What do you think about changing the files_to_ignore value to be comma-separated instead of whitespace so we can support filenames with whitespaces?

About the other limitations:

  • Global files_to_ignore: I would merge this as-is, and open an issue to support glob patterns
  • Only the first 100 files are taken into account: I would add this limitation to the readme and open an Issue

What do you think? 🙂

@rgomezcasas rgomezcasas self-assigned this Mar 17, 2022
@rgomezcasas rgomezcasas self-requested a review March 17, 2022 13:35
@favmd
Copy link
Contributor Author

favmd commented Mar 17, 2022

Amazing! I'll try it today.

Nice! That was fast! 👍

What do you think about changing the files_to_ignore value to be comma-separated instead of whitespace so we can support filenames with whitespaces?

About the other limitations:

  • Global files_to_ignore: I would merge this as-is, and open an issue to support glob patterns

  • Only the first 100 files are taken into account: I would add this limitation to the readme and open an Issue

What do you think? 🙂

@rgomezcasas You're totally right: The limitation that only the first 100 files are taken into account - when files_to_ignore is set - should definitely be mentioned prominently in the README.

Regarding the other limitations: I would like to suggest to merge this PR as-is and move all ideas into new ✨ feature issues, so that there is

  • a new issue to support filename globbing
  • a new issue to support pagination (up to 3000 files max)
  • a new issue to support a different IFS (with current default: files_to_ignore_ifs: ' ')

All of those changes could be implemented to be backward compatible.

Regarding the split character (will probably be implemented by setting the IFS in bash!?): Let's just make it configurable! Some people seem to prefer whitespaces (me), others want comma-separated (you), I can easily imagine that there's more out there, so I suggest a new issue with a new argument:

  • files_to_ignore_ifs: ' ' (default is whitespace to be backward compatible)

I would not try to quickfix this PR (too often those quickfixes add subtle bugs).

Let's go for new issues instead.

Sounds good?

@rgomezcasas rgomezcasas merged commit 0339fd2 into CodelyTV:master Mar 20, 2022
@rgomezcasas
Copy link
Member

Agree! Doing some tests in here before merging to main 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants