Added support to skip presubmit review via description tags #9

bstanga · 2024-11-18T10:02:38Z

Consider adding a log message before returning to explicitly indicate that the PR review is being skipped. This would improve observability and make it clearer in logs why no review was performed. For example:

if (shouldIgnorePullRequest(pull_request)) { info('Skipping PR review due to ignore tag in description'); return; }

bstanga · 2024-11-18T11:25:21Z

Consider adding a more detailed log message before returning to provide better context about why the PR review is being skipped. This would improve observability and debugging. For example:

if (shouldIgnorePullRequest(pull_request)) { info(`Skipping PR review due to ignore tag in description: ${pull_request.body}`); return; }

bstanga · 2024-11-18T12:03:42Z

The function call shouldIgnorePullRequest(pull_request) assumes that pull_request is properly defined. Consider adding a null check to handle cases where the pull request object might be undefined:

if (!pull_request) { error('Pull request object is undefined'); return; } if (shouldIgnorePullRequest(pull_request)) { return; }

bstanga · 2024-11-18T12:33:05Z

The code assumes pull_request is properly defined. Consider adding a null check to handle cases where the pull request object might be undefined:

if (!pull_request) { error('Pull request object is undefined'); return; } if (shouldIgnorePullRequest(pull_request)) { return; }

github-actions · 2024-11-18T09:27:49Z

Consider adding JSDoc documentation to describe the function's purpose, parameters, and return value. This will improve code maintainability and help other developers understand the function's behavior:

/** * Checks if the pull request should be ignored based on special commands in its description. * @param pull_request - The pull request object containing the description * @param pull_request.body - The description text of the pull request * @returns {boolean} True if the PR should be ignored, false otherwise */ function shouldIgnorePullRequest(pull_request: { body?: string }) {

github-actions · 2024-11-18T09:34:33Z

Consider using a more specific type for the pull_request parameter. Instead of using an inline type with just the body property, you could define a proper interface or type that better represents the pull request object. This would make the code more maintainable and type-safe. For example:

interface PullRequest { body?: string; // other relevant properties } function shouldIgnorePullRequest(pull_request: PullRequest)

bstanga · 2024-11-18T11:25:21Z

Add JSDoc documentation to describe the function's purpose, parameters, and return value. This will improve code maintainability and help other developers understand the function's behavior:

/** * Checks if the pull request should be ignored based on special commands in its description. * @param pull_request - The pull request object containing the description * @param pull_request.body - The description text of the pull request * @returns {boolean} True if the PR should be ignored, false otherwise */ function shouldIgnorePullRequest(pull_request: { body?: string }) {

github-actions · 2024-11-18T09:30:32Z

The current array of ignore phrases could be generated programmatically to reduce repetition and make maintenance easier. Consider refactoring to use a more structured approach:

const commands = ['ignore', 'skip']; const prefixes = ['@presubmit', '@presubmitai']; const separators = ['', ': ']; const ignorePhrases = prefixes.flatMap(prefix => commands.flatMap(command => separators.map(sep => `${prefix}${sep}${command}`) ) );

This approach makes it easier to add new commands, prefixes, or separator variations in the future while reducing the chance of typos.

Not a good comment

github-actions · 2024-11-18T09:34:33Z

Consider moving the ignore phrases to a configuration file. This would make it easier to maintain and modify the supported phrases without changing the code. You could create a config.ts file that exports these phrases:

// config.ts export const IGNORE_PHRASES = [ '@presubmit ignore', // ... other phrases ];

This approach would also make it easier to test and modify the supported phrases without touching the core logic.

bstanga · 2024-11-18T10:07:31Z

Instead of having a fixed list of phrases, consider using a regex pattern that would be more flexible and maintainable. This would also handle variations in whitespace more elegantly:

const IGNORE_REGEX = /^@presubmit(?:ai)?(?:\s*:\s*|\s+)(ignore|skip)$/im; function shouldIgnorePullRequest(pull_request: { body?: string }) { const body = pull_request.body ?? ""; const matches = body.split('\n').some(line => IGNORE_REGEX.test(line.trim())); if (matches) { info('ignoring pull request due to ignore/skip command in description'); } return matches; }

This approach:

Handles variations in whitespace more robustly

Matches commands only at the start of a line (preventing false positives)

Is more maintainable as it's a single pattern instead of multiple strings

Can be easily extended to support additional variations

bstanga · 2024-11-18T11:25:21Z

The current array of ignore phrases is repetitive and could be generated programmatically to reduce duplication and improve maintainability. Consider using a more structured approach:

const commands = ['ignore', 'skip']; const prefixes = ['@presubmit', '@presubmitai']; const separators = ['', ': ']; const ignorePhrases = prefixes.flatMap(prefix => commands.flatMap(command => separators.map(sep => `${prefix}${sep}${command}`) ) );

This makes it easier to add new commands or variations while reducing the chance of typos.

github-actions · 2024-11-18T09:32:47Z

While the nullish coalescing operator handles null and undefined, there's an unnecessary optional chaining operator (?.) used on line 300 when calling includes(). Since bodyLower is already guaranteed to be a string, you can remove the optional chaining:

if (bodyLower.includes(phrase.toLowerCase())) {

bstanga · 2024-11-18T11:25:21Z

The PR body text should be trimmed before checking for ignore phrases. Without trimming, extra whitespace could prevent matching valid ignore tags. Consider:

const bodyLower = (pull_request.body ?? "").toLowerCase().trim();

bstanga · 2024-11-18T12:33:05Z

The PR body text should be trimmed before checking for ignore phrases. Without trimming, extra whitespace at the start or end could prevent matching valid ignore tags. Consider:

const bodyLower = (pull_request.body ?? "").toLowerCase().trim();

bstanga · 2024-11-18T10:02:38Z

The PR body text should be trimmed before checking for ignore phrases. Without trimming, extra whitespace could prevent matching valid ignore tags. Consider:

const bodyLower = (pull_request.body ?? "").toLowerCase().trim();

bstanga · 2024-11-18T10:07:31Z

The toLowerCase() is being called on each phrase inside the loop, even though the phrases are constant strings. Consider converting the ignorePhrases array to lowercase when it's defined to avoid redundant string transformations:

const ignorePhrases = [ '@presubmit ignore', // ... other phrases ].map(phrase => phrase.toLowerCase()); // Then in the loop if (bodyLower.includes(phrase)) {

This way, the lowercase conversion happens only once per phrase instead of on every iteration.

github-actions · 2024-11-18T09:34:33Z

The function could be simplified using the some() array method, which would make the code more concise and functional. Here's a suggested refactoring:

return ignorePhrases.some(phrase => { const matches = bodyLower.includes(phrase.toLowerCase()); if (matches) { info(`ignoring pull request because of '${phrase}' in description`); } return matches; });

bstanga · 2024-11-18T11:25:22Z

The code could be simplified and made more efficient using the some() array method. Additionally, toLowerCase() is being called unnecessarily on each iteration for constant strings. Consider:

const ignorePhrases = [ '@presubmit ignore', // ... other phrases ].map(phrase => phrase.toLowerCase()); return ignorePhrases.some(phrase => { const matches = bodyLower.includes(phrase); if (matches) { info(`ignoring pull request because of '${phrase}' in description`); } return matches; });

bstanga · 2024-11-18T10:02:38Z

This new functionality should be accompanied by unit tests to verify the behavior of shouldIgnorePullRequest(). Tests should cover various scenarios including:

Different ignore phrase formats

Case sensitivity

Empty PR description

PR description with whitespace around ignore phrases

PR description without any ignore phrases

-Original file line number
+Diff line change
@@ Expand Up / @@ -33,6 +33,10 @@ export async function handlePullRequest() { @@
       const octokit = initOctokit(config.githubToken);
+      if (shouldIgnorePullRequest(pull_request)) {
+        return;
+      }
       // Get commit messages
       const { data: commits } = await octokit.rest.pulls.listCommits({
         ...context.repo,
@@ Expand Down Expand Up / @@ -302,3 +306,25 @@ async function submitReview( @@
         );
       }
     }
+    function shouldIgnorePullRequest(pull_request: { body?: string }) {
+      const ignorePhrases = [
+        "@presubmit ignore",
+        "@presubmit: ignore",
+        "@presubmit skip",
+        "@presubmit: skip",
+        "@presubmitai ignore",
+        "@presubmitai: ignore",
+        "@presubmitai skip",
+        "@presubmitai: skip",
+      ];
+      const bodyLower = (pull_request.body ?? "").toLowerCase();
+      for (const phrase of ignorePhrases) {
+        if (bodyLower.includes(phrase.toLowerCase())) {
+          info(`ignoring pull request because of '${phrase}' in description`);
+          return true;
+        }
+      }
+      return false;
+    }

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support to skip presubmit review via description tags #9

Diff view

Diff view

There are no files selected for viewing

bstanga Nov 18, 2024

bstanga Nov 18, 2024

bstanga Nov 18, 2024

bstanga Nov 18, 2024

github-actions bot Nov 18, 2024

github-actions bot Nov 18, 2024

bstanga Nov 18, 2024

github-actions bot Nov 18, 2024

bstanga Nov 18, 2024

github-actions bot Nov 18, 2024

bstanga Nov 18, 2024

bstanga Nov 18, 2024

github-actions bot Nov 18, 2024

bstanga Nov 18, 2024

bstanga Nov 18, 2024

bstanga Nov 18, 2024

bstanga Nov 18, 2024

github-actions bot Nov 18, 2024

bstanga Nov 18, 2024

bstanga Nov 18, 2024

Added support to skip presubmit review via description tags #9

Added support to skip presubmit review via description tags #9

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Nov 18, 2024

Choose a reason for hiding this comment

github-actions bot Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment