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

Use skip ignore comment instead of use ignore comment in run_rules #200

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

mvismonte
Copy link
Member

Summary

Currently, the run_rules command does not provide a way for us to ignore ignore comments. The default for the command is to respect them and the override flag provided just does the same. This PR replaces the option with the proper one that means that we are able to opt out if the --skip-ignore-comments is provided.

Test Plan

Setting up the test. Currently, there is a file in the repository that is missing a header file:

$ python -m fixit.cli.run_rules --rules AddMissingHeaderRule
Scanning 99 files
Testing 1 rules

/Users/markv/src/Fixit/.github/scripts/tox_job.py:1:1
    AddMissingHeaderRule: A required header comment for this file is missing.

Found 1 reports in 99 files in 4.57 seconds.

Let's add a suppression comment:

diff --git a/.github/scripts/tox_job.py b/.github/scripts/tox_job.py
index 9b5aac0..b01c678 100644
--- a/.github/scripts/tox_job.py
+++ b/.github/scripts/tox_job.py
@@ -1,3 +1,4 @@
+# lint-ignore: AddMissingHeaderRule
 """
 Generate the appropriate tox job name to match the current
 version of python available. For use with Github Actions

Now, let's run the lint again:

$  python -m fixit.cli.run_rules --rules AddMissingHeaderRule
Scanning 99 files
Testing 1 rules


Found 0 reports in 99 files in 4.55 seconds.

There are 0 reports, as expected. Now, let's check to see if the --skip-ignore-comments flag works:

$ python -m fixit.cli.run_rules --rules AddMissingHeaderRule --skip-ignore-comments
Scanning 99 files
Testing 1 rules

/Users/markv/src/Fixit/.github/scripts/tox_job.py:1:1
    AddMissingHeaderRule: A required header comment for this file is missing.

Found 1 reports in 99 files in 2.59 seconds.

It does! Previously, there would no way to suppress the ignore comment. Help provided the following:

$ python -m fixit.cli.run_rules --rules AddMissingHeaderRule --help
.
.
.
  --use-ignore-comments
                        Obey `# noqa`, `# lint-fixme` and `# lint-ignore`
                        comments.
.
.
.

However, running the command without anything shows that we are respecting ignore comments:

$  python -m fixit.cli.run_rules --rules AddMissingHeaderRule
Scanning 99 files
Testing 1 rules


Found 0 reports in 99 files in 4.95 seconds.

Providing the --use-ignore-comments doesn't make sense because we want to ignore/skip them, but let's try anyways:

$  python -m fixit.cli.run_rules --rules AddMissingHeaderRule --use-ignore-comments
Scanning 99 files
Testing 1 rules


Found 0 reports in 99 files in 5.03 seconds.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 30, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #200 (b83d444) into master (ad655b2) will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
- Coverage   86.19%   86.11%   -0.09%     
==========================================
  Files          91       91              
  Lines        3744     3744              
==========================================
- Hits         3227     3224       -3     
- Misses        517      520       +3     
Impacted Files Coverage Δ
fixit/cli/run_rules.py 41.89% <ø> (ø)
fixit/cli/args.py 70.58% <0.00%> (-3.53%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad655b2...b83d444. Read the comment docs.

@lisroach
Copy link
Contributor

Thank you!

@lisroach lisroach merged commit 2486a88 into Instagram:master Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants