fix: improve resilience of comment searching (#39)

* fix: improve resilience of comment searching * Improve this more * Update package.json * clean up changes * Improve comment * Fix es warnings and add unit test
slackapi · Aug 6, 2024 · 77dcac0 · 77dcac0
1 parent a06beee
commit 77dcac0
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 7 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -64,6 +64,16 @@ module.exports = {
     // Allow safe references to functions before the declaration. Overrides AirBnB config. Not located in the override
     // section below because a distinct override is necessary in TypeScript files.
     'no-use-before-define': ['error', 'nofunc'],
+
+    // Allow unused variables if they start with an underscore
+    'no-unused-vars': [
+      'error',
+      {
+        argsIgnorePattern: '^_',
+        varsIgnorePattern: '^_',
+        caughtErrorsIgnorePattern: '^_',
+      },
+    ],
   },
 
   overrides: [

diff --git a/src/score_components/find-problematic-comments.js b/src/score_components/find-problematic-comments.js
@@ -1,6 +1,12 @@
 const fs = require('fs');
 const child_process = require('child_process');
 
+const CommentType = Object.freeze({
+  TODO: 'TODO',
+  FIXME: 'FIXME',
+  HACK: 'HACK',
+});
+
 module.exports = function grepForProblematicComments(core, ext, include, exclude) {
   let find = 'find';
   if (include && include.length) {
@@ -31,7 +37,7 @@ module.exports = function grepForProblematicComments(core, ext, include, exclude
       find += ` -not -path "*/${ex}"`;
     }
   });
-  const commentPattern = '\\s*(//|/\\*|\\*).*\\b(TODO|HACK|FIXME)\\b';
+  const commentPattern = `\\s*(//|/\\*|\\*).*\\b(${CommentType.TODO}|${CommentType.HACK}|${CommentType.FIXME})\\b`;
 
   // Modify the grep command to search within comments only
   find += ` -exec sh -c 'grep -EHn "${commentPattern}" "$0"' {} \\;`;
@@ -43,12 +49,35 @@ module.exports = function grepForProblematicComments(core, ext, include, exclude
     core.error('child_process execSync failed to execute');
   }
   const result = output.split('\n').filter(Boolean).map((line) => {
-    const [path, lineNo, type, commentData] = line.split(':');
-    if (type) type.trim();
-    if (commentData) commentData.trim();
-    const comment = (commentData == null) ? type.trim() : (`${type.trim()}: ${commentData.trim()}`).trim();
-    const commentType = (commentData == null) ? null : type.split('//')[1].trim();
-    return { path: path.trim(), line_no: parseInt(lineNo, 10), comment, commentType };
+    // example line output = "./src/report.js:47:  // TODO: handle API call erroring out"
+    const [path, lineNo, ...data] = line.split(':');
+
+    const lineData = data.join(':');
+    const [_code, ...rawCommentData] = lineData.split('//');
+
+    const commentData = rawCommentData.join('//');
+    const commentType = getCommentType(commentData);
+
+    return { path: path.trim(), line_no: parseInt(lineNo, 10), comment: `//${commentData}`.trim(), commentType };
   });
   return result;
 };
+
+/**
+ * Get the comment type.
+ * @param {string} comment
+ * @returns {string|null} comment type or null
+ */
+function getCommentType(comment) {
+  const cleanComment = comment.trim();
+  if (cleanComment.startsWith(CommentType.TODO)) {
+    return CommentType.TODO;
+  }
+  if (cleanComment.startsWith(CommentType.HACK)) {
+    return CommentType.HACK;
+  }
+  if (cleanComment.startsWith(CommentType.FIXME)) {
+    return CommentType.FIXME;
+  }
+  return null;
+}
diff --git a/test/score_components/problematic-comments-test.js b/test/score_components/problematic-comments-test.js
@@ -77,5 +77,15 @@ describe('score component: problematic comments', () => {
       const score = { comments: grepForProblematicComments(fakeCore, ['js'], ['src'], []), coverageMisses: 0 };
       assert.deepEqual(score.comments, [{ path: 'path', line_no: 10, comment: '// TODO: random stuff', commentType: 'TODO' }, { path: 'path2', line_no: 15, comment: '// random things TODO', commentType: null }]);
     });
+    it('should handle grep results that have types without colons', async () => {
+      fakeChildProcess.execSync.returns('path:10: // TODO random stuff');
+      const score = { comments: grepForProblematicComments(fakeCore, ['js'], ['src'], []), coverageMisses: 0 };
+      assert.deepEqual(score.comments, [{ path: 'path', line_no: 10, comment: '// TODO random stuff', commentType: 'TODO' }]);
+    });
+    it('should handle grep results that starts with code', async () => {
+      fakeChildProcess.execSync.returns('path:10: const result = "hello:world" // TODO: random stuff');
+      const score = { comments: grepForProblematicComments(fakeCore, ['js'], ['src'], []), coverageMisses: 0 };
+      assert.deepEqual(score.comments, [{ path: 'path', line_no: 10, comment: '// TODO: random stuff', commentType: 'TODO' }]);
+    });
   });
 });