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

Not possible to specify --force-exclude in Github Action #1225

Open
christianharrington opened this issue Feb 6, 2025 · 9 comments
Open
Labels
A-gh-action C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing

Comments

@christianharrington
Copy link

I'd like to only check files that have changed compared to the merge base.
Using git, I can pass these into the Github Action using the files input.
However, this also checks files excluded in the configuration file.

It would be very useful if you could set the --force-exclude flag using the Action.

If you think this is a good idea, I can create a PR that adds this input to action.yml and updates action/entrypoint.sh to read it. As far as I can tell, that's the only thing that needs updating.

An alternative approach could be to add an input to the Action that can be used to pass arbitrary arguments on to typos. This is a bit more flexible, but also easier to use incorrectly.

@epage
Copy link
Collaborator

epage commented Feb 6, 2025

What is the reason you are wanting to limit the checked files?

If we move forward with this, my question is if we should unconditionally set it for the action, like we do for pre-commit.

@christianharrington
Copy link
Author

What is the reason you are wanting to limit the checked files?

It's because we have several classes of files that we have added to the exclude list, as they are expected to have spelling errors. This could be assets that are translated to other languages, generated files, etc.

If someone makes changes to these files, they are included in the list of files to check for typos, and show up in the PR as having typos.

@christianharrington
Copy link
Author

If we move forward with this, my question is if we should unconditionally set it for the action, like we do for pre-commit.

This would definitely work for my use-case 👍

@epage
Copy link
Collaborator

epage commented Feb 10, 2025

Forgot to link to past attempts

What is the reason you are wanting to limit the checked files?

It's because we have several classes of files that we have added to the exclude list, as they are expected to have spelling errors. This could be assets that are translated to other languages, generated files, etc.

Sorry, I wasn't clear. What is your reason that you are wanting to only check files compared to the merge base?

e.g. #1096 was looking to use it for performance gains (which it ended up there weren't any) and to build a custom exclusion system on top of typos (which the design tries to discourage so people can run it locally).

@christianharrington
Copy link
Author

What is your reason that you are wanting to only check files compared to the merge base?

It's because I'm trying to introduce typos to a large code-base with more than 10 years of accumulated typos 😅

It's complicated by the fact that some of the typos have made it into public interfaces, as well as quite a few integrations to local systems that use non-English words. Combined, this means that there's many spelling errors that can't be fixed, and many false positives (as the words aren't in English). Fixing all these in one go doesn't seem worth it 🤷

However, I'd still like to stop the introduction of new spelling errors, so we use the action to warn on any spelling errors in changed files.

I can imagine other people in similar situations, but if supporting this use-case isn't desirable, I can also just fork with the added parameter (or just call typos directly, without using the action) 😄

@epage
Copy link
Collaborator

epage commented Feb 11, 2025

Are you familiar with Github's SARIF support? I've not tested this part of it out but from my understanding, it keeps a database of previously seen lints and only reports lines that are new with a PR. This seems perfect for the kind of scenario you are in. We gained support for SARIF in #1047 but haven't fully integrated into our action yet (tracking that in #1036).

@christianharrington
Copy link
Author

Are you familiar with Github's SARIF support? I've not tested this part of it out but from my understanding, it keeps a database of previously seen lints and only reports lines that are new with a PR.

I've read about it previously, but never attempted to use it. I wasn't aware of the functionality where it only shows new alerts.

However, the documentation for SARIF support makes me think it may not be available to all repos on Github:

Who can use this feature?
Code scanning is available for the following repository types:

  • Public repositories on GitHub.com
  • Organization-owned repositories on GitHub Enterprise Cloud with GitHub Advanced Security enabled

For my specific case, I'd personally prefer not to introduce another mechanism (Github's SARIF support) in order to only show warnings for changed files, when there's already a familiar git-based solution that can be used for many different use-cases.

@epage
Copy link
Collaborator

epage commented Feb 13, 2025

For my specific case, I'd personally prefer not to introduce another mechanism (Github's SARIF support) in order to only show warnings for changed files, when there's already a familiar git-based solution that can be used for many different use-cases.

A git based solution also has downsides as noted in #1096 which includes

  • How do you handle new typos found on upgrade as only changed files will be checked on upgrade? You could consider them part of your backlog but that won't work as well when there is no more backlog
  • This couples changing a file, for any reason, with responding to typos
  • For some of the "many different use cases": running typos locally will fail if this is overly relied on

@christianharrington
Copy link
Author

I agree that the points you mention would be problems if you didn't allow merging a PR in the presence of typos:

You'd be forced to fix all existing typos in any file you touched, and the job could break if you upgraded without verifying that this didn't cause additional typos to be caught.

However, in my use-case, where I'm trying to introduce typos incrementally, we still allow merging PRs even in the presence of typos. We use the Github annotation support to highlight any typos in files being changed.

Anecdotally, based on doing a lot of code review, this has both prevented a bunch of new typos from being merged, as well as encouraged people to fix existing typos when making changes to files.

To me, this seems like a reasonable way to gradually introduce a new linting tool to a large existing repository, and thought others might want to use the action to do it in a similar way.

Like I said earlier, it's pretty straight-forward to accomplish with either a fork of the action or by just calling typos directly, if you think the additional input is too easy to misuse.

@epage epage added A-gh-action C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing labels Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-gh-action C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Development

No branches or pull requests

2 participants