Skip to content

chore: new default exclude patterns for .kts & tests - #13

Merged
Qiyuanqiii merged 1 commit into
Qiyuanqiii:codex/issue-1090-kts-kotlin-rulesfrom
MacTrust:codex/issue-1090-kts-kotlin-rules
Aug 28, 2026
Merged

chore: new default exclude patterns for .kts & tests#13
Qiyuanqiii merged 1 commit into
Qiyuanqiii:codex/issue-1090-kts-kotlin-rulesfrom
MacTrust:codex/issue-1090-kts-kotlin-rules

Conversation

@wu21-web

@wu21-web wu21-web commented Aug 27, 2026

Copy link
Copy Markdown

Description

chore: new default exclude patterns for .kts & tests
Recommended mode: rebase

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • [-] I have signed the CLA

@wu21-web
wu21-web force-pushed the codex/issue-1090-kts-kotlin-rules branch from a8ed328 to 6ecf881 Compare August 27, 2026 15:14
@Qiyuanqiii

Copy link
Copy Markdown
Owner

Thanks for taking the time to review alibaba#1091 and for preparing this follow-up PR. I agree that keeping the default exclusion behavior consistent between .kt and .kts is worth considering.

After reviewing the changes, however, I think this should be submitted and reviewed as a separate PR against the upstream main branch rather than merged into alibaba#1091.

The scope of alibaba#1091 is limited to rule resolution:

This PR changes a different stage of the review pipeline. Specifically, it changes:

**/src/test/**/*.kt

to:

**/src/test/**/*.{kt,kts}

That means .kts files under src/test/** would be excluded before rule resolution takes place. This may be a reasonable consistency improvement, but it changes observable file-selection behavior: some Kotlin script files that are currently eligible for review would no longer be reviewed by default.

Keeping the two changes separate would allow maintainers to evaluate two independent questions clearly:

  1. Should an eligible .kts file use the existing Kotlin review rules? This is what fix(rules): route Kotlin script files to Kotlin review rules alibaba/open-code-review#1091 addresses.
  2. Should .kts files under src/test/** be excluded by default, in the same way as .kt files? This is what your PR addresses.

It also keeps alibaba#1091 atomic and aligned with its title and description: fixing rule routing without silently changing the default exclusion policy. PR alibaba#1095 follows the same boundary for Scala by adding language-specific rule resolution while explicitly leaving file eligibility and default exclusions unchanged. alibaba#1095 is not a dependency of this proposal, but it demonstrates why keeping rule routing and file-selection policy separate makes the changes easier to review and revert independently.

Would you mind opening this as a separate PR directly against alibaba/open-code-review:main? The PR could focus on the exclusion-pattern change and its regression tests, with alibaba#1091 linked only as related context. Rebasing or cherry-picking your existing commit onto an upstream-based branch would also preserve your authorship.

For this reason, I would prefer not to merge #13 into the alibaba#1091 feature branch. This is not a rejection of the proposal—only a request to have the file-selection behavior change reviewed independently.

Thanks again for identifying the inconsistency and adding the test coverage.

@wu21-web

Copy link
Copy Markdown
Author

@Qiyuanqiii My last note: update the docs if nesscary.

@wu21-web wu21-web closed this Aug 27, 2026
@wu21-web
wu21-web deleted the codex/issue-1090-kts-kotlin-rules branch August 28, 2026 00:06
@wu21-web
wu21-web restored the codex/issue-1090-kts-kotlin-rules branch August 28, 2026 00:07
@wu21-web wu21-web reopened this Aug 28, 2026
@Qiyuanqiii
Qiyuanqiii merged commit ca60521 into Qiyuanqiii:codex/issue-1090-kts-kotlin-rules Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants