Merge pull request #21 from ethanis/use-actions-glob

Correctly filter out exclusion paths

.vscode/launch.json

@@ -0,0 +1,17 @@
+{
+  // Use IntelliSense to learn about possible attributes.
+  // Hover to view descriptions of existing attributes.
+  // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
+  "version": "0.2.0",
+  "configurations": [
+    {
+      "name": "Debug Jest Tests",
+      "type": "node",
+      "request": "launch",
+      "program": "${workspaceFolder}/node_modules/.bin/jest",
+      "args": ["--runInBand"],
+      "console": "integratedTerminal",
+      "internalConsoleOptions": "neverOpen"
+    }
+  ]
+}

__tests__/services/comments.test.ts

@@ -1,8 +1,6 @@
-import * as path from 'path';
 import { getCommentBody } from '../../src/services';
-import { Constants } from '../../src/constants';
 
-test('read valid config file', () => {
+test('link to md5 of filepath', () => {
   const markdown = 'markdown';
   const files = ['package.json', 'src/services/comments.ts'];
   const prNumber = 42;

__tests__/services/state.test.ts

@@ -0,0 +1,166 @@
+import { getMatchingFilePaths } from '../../src/services';
+import { Comment, Change, ChangeType } from '../../src/models';
+
+test('* matches everything', () => {
+  const comment: Comment = {
+    pathFilter: ['*'],
+    markdown: 'Woohoo!',
+    blocking: false
+  };
+
+  const changes: Change[] = [
+    {
+      file: 'app/models/foo.rb',
+      changeType: ChangeType.edit
+    }
+  ];
+
+  const result = getMatchingFilePaths(comment, changes);
+
+  expect(result).toEqual(changes.map(c => c.file));
+});
+
+test('match recursively', () => {
+  const comment: Comment = {
+    pathFilter: ['app/**'],
+    markdown: 'Woohoo!',
+    blocking: false
+  };
+
+  const changes: Change[] = [
+    {
+      file: 'app/models/foo.rb',
+      changeType: ChangeType.edit
+    }
+  ];
+
+  const result = getMatchingFilePaths(comment, changes);
+
+  expect(result).toEqual(changes.map(c => c.file));
+});
+
+test('ignore case', () => {
+  const comment: Comment = {
+    pathFilter: ['app/**'],
+    markdown: 'Woohoo!',
+    blocking: false
+  };
+
+  const changes: Change[] = [
+    {
+      file: 'APP/MODELS/FOO.rb',
+      changeType: ChangeType.edit
+    }
+  ];
+
+  const result = getMatchingFilePaths(comment, changes);
+
+  expect(result).toEqual(changes.map(c => c.file));
+});
+
+test('remove exclusion patterns', () => {
+  const comment: Comment = {
+    pathFilter: ['app/**', '!app/models/*.h'],
+    markdown: 'Everything except headers',
+    blocking: false
+  };
+
+  const changes: Change[] = [
+    {
+      file: 'app/models/main.h',
+      changeType: ChangeType.edit
+    }
+  ];
+
+  const result = getMatchingFilePaths(comment, changes);
+
+  expect(result).toEqual([]);
+});
+
+test('match new files only', () => {
+  const comment: Comment = {
+    pathFilter: ['+app/**'],
+    markdown: 'Only new files',
+    blocking: false
+  };
+
+  const changes: Change[] = [
+    {
+      file: 'app/models/old.rb',
+      changeType: ChangeType.edit
+    },
+    {
+      file: 'app/models/new.rb',
+      changeType: ChangeType.add
+    }
+  ];
+
+  const result = getMatchingFilePaths(comment, changes);
+
+  expect(result).toEqual(['app/models/new.rb']);
+});
+
+test('match deleted files only', () => {
+  const comment: Comment = {
+    pathFilter: ['-app/**'],
+    markdown: 'Only deleted files',
+    blocking: false
+  };
+
+  const changes: Change[] = [
+    {
+      file: 'app/models/old.rb',
+      changeType: ChangeType.edit
+    },
+    {
+      file: 'app/models/deleted.rb',
+      changeType: ChangeType.delete
+    }
+  ];
+
+  const result = getMatchingFilePaths(comment, changes);
+
+  expect(result).toEqual(['app/models/deleted.rb']);
+});
+
+test('match edited files only', () => {
+  const comment: Comment = {
+    pathFilter: ['~app/**'],
+    markdown: 'Only deleted files',
+    blocking: false
+  };
+
+  const changes: Change[] = [
+    {
+      file: 'app/models/old.rb',
+      changeType: ChangeType.edit
+    },
+    {
+      file: 'app/models/deleted.rb',
+      changeType: ChangeType.delete
+    }
+  ];
+
+  const result = getMatchingFilePaths(comment, changes);
+
+  expect(result).toEqual(['app/models/old.rb']);
+});
+
+test('disallow multiple modifiers', () => {
+  const comment: Comment = {
+    pathFilter: ['!~app/**'],
+    markdown: 'Not edited files',
+    blocking: false
+  };
+
+  const changes: Change[] = [
+    {
+      file: 'app/models/old.rb',
+      changeType: ChangeType.edit
+    }
+  ];
+
+  expect(() => getMatchingFilePaths(comment, changes)).toThrow(
+    'Multiple path modifiers are not supported'
+  );
+});

src/services/state.ts

@@ -12,6 +12,8 @@ import { getExistingComments } from '.';
 import { IOptions, Minimatch } from 'minimatch';
 import { Constants } from '../constants';
 
+const pathModifiers = ['!', '+', '-', '~'];
+
 export async function getTargetState(
   octokit: github.GitHub,
   allComments: Comment[],
@@ -33,7 +35,7 @@ export async function getTargetState(
   );
 
   for (const comment of allComments) {
-    const matches = isCommentApplicable(comment, changes);
+    const matches = getMatchingFilePaths(comment, changes);
     const isApplicable = matches.length > 0;
 
     const commentMatchText = `${comment.markdown}${Constants.CannedTextSeparator}`;
@@ -75,7 +77,10 @@ export async function getTargetState(
   };
 }
 
-function isCommentApplicable(comment: Comment, changes: Change[]): string[] {
+export function getMatchingFilePaths(
+  comment: Comment,
+  changes: Change[]
+): string[] {
   const results: string[] = [];
   const options: IOptions = { dot: true, nocase: true };
 
@@ -88,6 +93,10 @@ function isCommentApplicable(comment: Comment, changes: Change[]): string[] {
     } else {
       inclusions.push(pathFilter);
     }
+
+    if (pathModifiers.includes(pathFilter[1])) {
+      throw new Error('Multiple path modifiers are not supported');
+    }
   }
 
   for (const change of changes) {
@@ -149,7 +158,7 @@ function isCommentApplicable(comment: Comment, changes: Change[]): string[] {
     }
 
     // Check if any exclusion should filter out the match
-    for (const exclusion in exclusions) {
+    for (const exclusion of exclusions) {
       // First exclusion to match will negate the inclusion match
       const matcher = new Minimatch(exclusion, options);
       const match = matcher.match(change.file);