Skip to content

[code-simplifier] refactor: simplify hasSpecialTriggers to a single boolean expression#22837

Merged
pelikhan merged 1 commit intomainfrom
simplify/concurrency-has-special-triggers-58f3367dc769a200
Mar 25, 2026
Merged

[code-simplifier] refactor: simplify hasSpecialTriggers to a single boolean expression#22837
pelikhan merged 1 commit intomainfrom
simplify/concurrency-has-special-triggers-58f3367dc769a200

Conversation

@github-actions
Copy link
Contributor

Simplifies hasSpecialTriggers in pkg/workflow/concurrency.go, recently added in #22825.

Files Simplified

  • pkg/workflow/concurrency.go — Replaced 6 sequential if/return true blocks with a single idiomatic return using boolean OR

Improvement

The original implementation used a pattern common in early drafts — checking each condition individually and returning true from each — but Go idiom favors a direct boolean expression for this kind of "is any of these true?" query:

Before (36 lines including comments):

if isIssueWorkflow(on) {
    return true
}
if isPullRequestWorkflow(on) {
    return true
}
// ... 4 more identical blocks
return false

After (7 lines):

return isIssueWorkflow(on) ||
    isPullRequestWorkflow(on) ||
    isDiscussionWorkflow(on) ||
    isPushWorkflow(on) ||
    isSlashCommandWorkflow(on) ||
    isWorkflowDispatchOnly(on)

Changes Based On

Recent change from #22825 which introduced the hasBotSelfCancelRisk and related concurrency helpers.

Testing

  • ✅ All TestHasSpecialTriggers subtests pass (all 9 cases)
  • ✅ All TestHasBotSelfCancelRisk subtests pass
  • ✅ All TestBotActorIsolationInConcurrencyKeys subtests pass
  • ✅ All TestGenerateConcurrencyConfigWithBotIsolation subtests pass
  • ✅ Build succeeds (make build)
  • ✅ No functional changes — behavior is identical

Review Focus

Please verify the boolean expression covers exactly the same trigger types as the original if-chain.

Note

🔒 Integrity filter blocked 1 item

The following item were blocked because they don't meet the GitHub integrity level.

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by Code Simplifier ·

  • expires on Mar 26, 2026, 6:04 AM UTC

Replace 6 if/return-true blocks with a single idiomatic return
statement using boolean OR. The behavior is identical — each
trigger type check is unchanged — but the control flow is now
much easier to read at a glance.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Contributor Author

Hey @github-actions 👋 — nice work from the Code Simplifier on collapsing the hasSpecialTriggers if-chain in pkg/workflow/concurrency.go into a single idiomatic Go boolean expression. The PR is well-scoped, touch-free on deps, and comes with a clear before/after description.

One thing that would lift this to lgtm: the diff doesn't include any changes to test files. The PR body notes all 9 TestHasSpecialTriggers cases pass in CI, which is great — but the existing tests don't explicitly document that the refactored single expression is exhaustive (i.e., that each of the 6 trigger-type functions independently causes the function to return true). Adding or updating a subtest in pkg/workflow/concurrency_test.go would make the behavioral contract visible at the test level, not just in CI logs.

If you'd like a hand, here's a prompt to address this:

In pkg/workflow/concurrency_test.go, update the TestHasSpecialTriggers test suite to add a subtest
(or table row) for each of the six trigger-type functions called in the refactored hasSpecialTriggers:
  - isIssueWorkflow
  - isPullRequestWorkflow
  - isDiscussionWorkflow
  - isPushWorkflow
  - isSlashCommandWorkflow
  - isWorkflowDispatchOnly

Each subtest should verify that a WorkflowData whose On field activates exactly one of these
functions causes hasSpecialTriggers to return true, and that a WorkflowData with none of them
(e.g., a schedule-only workflow) returns false.

This ensures the refactored single-expression return is provably equivalent to the original
if-chain and documents the exhaustive coverage at the test level.

Generated by Contribution Check ·

@pelikhan pelikhan marked this pull request as ready for review March 25, 2026 10:33
Copilot AI review requested due to automatic review settings March 25, 2026 10:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors hasSpecialTriggers to use a single idiomatic boolean OR expression while preserving the same trigger checks used to decide whether to skip default job-level concurrency.

Changes:

  • Replaced a sequential if { return true } chain with a single return a || b || ... expression in hasSpecialTriggers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pelikhan pelikhan merged commit 52d3cea into main Mar 25, 2026
64 checks passed
@pelikhan pelikhan deleted the simplify/concurrency-has-special-triggers-58f3367dc769a200 branch March 25, 2026 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants