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

fix: improve resilience of comment searching #39

Merged
merged 6 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
Expand Down
43 changes: 36 additions & 7 deletions src/score_components/find-problematic-comments.js
Original file line number Diff line number Diff line change
@@ -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) {
Expand Down Expand Up @@ -31,7 +37,7 @@
find += ` -not -path "*/${ex}"`;
}
});
const commentPattern = '\\s*(//|/\\*|\\*).*\\b(TODO|HACK|FIXME)\\b';
const commentPattern = `\\s*(//|/\\*|\\*).*\\b(${CommentType.TODO}|${CommentType.HACK}|${CommentType.FIXME})\\b`;

Check warning on line 40 in src/score_components/find-problematic-comments.js

View workflow job for this annotation

GitHub Actions / Health Score

src/score_components/find-problematic-comments.js#L40

Problematic comment ("TODO", "HACK", "FIXME") identified

// Modify the grep command to search within comments only
find += ` -exec sh -c 'grep -EHn "${commentPattern}" "$0"' {} \\;`;
Expand All @@ -43,12 +49,35 @@
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"

Check warning on line 52 in src/score_components/find-problematic-comments.js

View workflow job for this annotation

GitHub Actions / Health Score

src/score_components/find-problematic-comments.js#L52

Problematic comment ("TODO", "HACK", "FIXME") identified
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
filmaj marked this conversation as resolved.
Show resolved Hide resolved
* @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;
}

Check warning on line 78 in src/score_components/find-problematic-comments.js

View check run for this annotation

Codecov / codecov/patch

src/score_components/find-problematic-comments.js#L77-L78

Added lines #L77 - L78 were not covered by tests
if (cleanComment.startsWith(CommentType.FIXME)) {
return CommentType.FIXME;
}
return null;
}
10 changes: 10 additions & 0 deletions test/score_components/problematic-comments-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' }]);
});
});
});
Loading