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

Support content filters that only post a comment when a regex is not found #33

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
7 changes: 7 additions & 0 deletions .github/nitpicks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,10 @@
blocking: true
pathFilter:
- '+**/*.dll'

- markdown: |
Testing - fail if the file doesn't include "always()"
pathFilter:
- '.github/workflows/example.yml'
contentFilter:
- '!always()'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think non-matching content filters should be their own input? It might be a valid use case for one to want to match a string that does start with a ! 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we can do it that way too! I personally felt like these were two sides of the same coin so it made sense to use contentFilter for both

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll run a test to see what '!(!always())' will do as a content filter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That actually appears to work. I ran a test with this as the content filter:

 contentFilter:
    - '!(!always())'

with this as the yaml, it produced a comment:
if: 'always()'

with this as the yaml, it did not produce a comment:
if: '!always()'

21 changes: 21 additions & 0 deletions .github/workflows/example.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
name: Build
on:
pull_request:
push:
branches:
- master
- 'releases/*'

jobs:
build-and-test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v1

- run: npm ci
- run: npm run all
- name: nitpicker
uses: ./
with:
nitpicks: './.github/nitpicks.yml'
token: '${{ secrets.GITHUB_TOKEN }}'
11 changes: 11 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,17 @@ A comment's `contentFilter` parameter accepts an array of regular expressions th
- '(\+\s*debugger)'
```

If you would like nitpicker to only post a comment when the regular expression is _not_ found, you can use a `!` in front of the contentFilter. For example:

```yaml
- markdown: |
Please ensure that all .ts files include the word 'unicorn!'
pathFilter:
- 'src/**/*.ts'
contentFilter:
- '!(unicorn)'
```

## Development

Install the dependencies
Expand Down
9 changes: 9 additions & 0 deletions __tests__/configReader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ test('contentFilter defaults to undefined', () => {
expect(comments.every(c => !c.contentFilter)).toBeTruthy();
});

test('contentFilter is negated', () => {
const configFile = './data/default_content_filter.yml';
const inputPath = path.join(__dirname, configFile);

const comments = getConfiguredComments(inputPath);

expect(comments.every(c => !c.contentFilter)).toBeTruthy();
});

test('read invalid config file', () => {
const configFile = './data/invalid_nitpicks.yml';
const inputPath = path.join(__dirname, configFile);
Expand Down
8 changes: 8 additions & 0 deletions __tests__/data/negated_content_filter.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
- markdown: |
## Notice
Thanks for this contribution. We _really_ appreciate your help!

- markdown: |
## Blocking issue
contentFilter:
- '!(\+\s*console.log\(.*\))'
46 changes: 46 additions & 0 deletions __tests__/services/state.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,52 @@ test('getMatchingContentChanges returns single changes with multiple matching co
expect(result).toHaveLength(1);
});

test('getMatchingContentChanges returns single changes with negated contentFilter', () => {
const comment: Comment = {
pathFilter: ['*'],
markdown: 'Everything',
blocking: false,
contentFilter: [`!(\\+\\s*debugger)`]
};

const changes: Change[] = [
{
file: 'src/services/configReader.ts',
changeType: ChangeType.edit,
patch:
"@@ -4,14 +4,19 @@ import { safeLoad } from 'js-yaml';\n" +
" import { Constants } from '../constants';\n" +
' \n' +
' export function getConfiguredComments(filePath: string): Comment[] {\n' +
'+ if (!fs.existsSync(filePath)) {\n' +
'+ throw Error(`Nitpicks file does not exist: ${filePath}`);\n' +
'+ }\n' +
'+\n' +
" const contents = fs.readFileSync(filePath, 'utf8');\n" +
' const yaml: Comment[] = safeLoad(contents);\n' +
' \n' +
'+ nope!\n' +
'+ console.log("this shouldnt have been committed!")\n' +
' \n' +
' const comments: Comment[] = yaml\n' +
'- .filter(y => y.markdown && y.pathFilter?.length > 0)\n' +
'+ .filter(y => y.markdown)\n' +
' .map(c => ({\n' +
' ...c,\n' +
"- markdown: `${c.markdown}${c.blocking ? Constants.BlockingText : ''}`\n" +
"+ markdown: `${c.markdown}${c.blocking ? Constants.BlockingText : ''}`,\n" +
'+ pathFilter: c.pathFilter ?? Constants.DefaultPathFilter // Default to match everything\n' +
' }));\n' +
' \n' +
' return comments;'
}
];

const result = getMatchingContentChanges(comment, changes);

expect(result).toHaveLength(1);
});

test('getMatchingContentChanges throws error if unable to parse regexpr', () => {
const comment: Comment = {
pathFilter: ['*'],
Expand Down
Loading