Skip to content

Step 1 (PR-D): DetachedCriteriaSpec command-chain syntax cleanup#15684

Open
jamesfredley wants to merge 1 commit into
8.0.xfrom
chore/tck-detached-criteria-style
Open

Step 1 (PR-D): DetachedCriteriaSpec command-chain syntax cleanup#15684
jamesfredley wants to merge 1 commit into
8.0.xfrom
chore/tck-detached-criteria-style

Conversation

@jamesfredley
Copy link
Copy Markdown
Contributor

Step 1 (PR-D) prerequisite for Hibernate 7 work - extracted from the staging branch so it can be reviewed on its own merits.

Context

This change was originally pulled forward into the hibernate7 staging branch as part of PR #15654. @sbglasius flagged it on DetachedCriteriaSpec.groovy:

"This is an unneeded change for this PR. Removing parentheses should have been done in a clean-up PR."

Extracting it here against 8.0.x so it can land on its own and then flow naturally into both the staging branch (PR #15654) and Step 2 (PR #15568).

Scope

Single file. 18 line changes. Pure style cleanup:

// before
criteria.with {
    eq('lastName', 'Simpson')
}

// after
criteria.with {
    eq 'lastName', 'Simpson'
}
Call Count
eq('lastName', 'Simpson')eq 'lastName', 'Simpson' 15
like('firstName', 'B%')like 'firstName', 'B%' 3
Total 18

This matches the idiomatic Groovy DSL convention used elsewhere in GORM criteria builders and is the form adopted on the hibernate7 staging branch.

Why a separate PR

PR #15654 (Step 1) is meant to be a near-pure clone of hibernate5hibernate7. PR #15568 (Step 2) is the actual Hibernate 7 logic. @sbglasius's review of the staging PR asked for cleanup of this nature to be split out so the review effort on Step 1 and Step 2 can stay focused on hibernate-related diffs.

Once this PR is merged, the corresponding style change on the hibernate7 staging branch should be removed since the change will arrive through the next merge of 8.0.x.

Related

Replaces 18 method calls of the form `eq('field', 'value')` and
`like('field', 'pattern')` inside `criteria.with { ... }` and
`criteria.build { ... }` closures with the Groovy command-chain form
`eq 'field', 'value'` / `like 'field', 'pattern'`.

This matches the idiomatic Groovy DSL style used elsewhere in GORM
criteria builders and was the convention adopted in the hibernate7
staging branch.

This change was originally pulled forward into the hibernate7 staging
branch as part of a larger PR #15654 commit and was flagged by
@sbglasius as belonging in a separate clean-up PR rather than mixed
with the Hibernate 7 clone. Extracting it here so it can land on 8.0.x
on its own merits as a prerequisite for the Hibernate 7 work.

Once merged here, the corresponding style change on the hibernate7
staging branch should be removed since it will arrive through the
next merge of 8.0.x.

Assisted-by: claude-code:claude-4.7-opus
Copilot AI review requested due to automatic review settings May 27, 2026 21:17
Copy link
Copy Markdown
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

Style-only cleanup to make DetachedCriteriaSpec use Groovy command-chain syntax for criteria DSL calls, aligning the TCK tests with the idiomatic GORM criteria style (and keeping the Hibernate 7 prerequisite work focused).

Changes:

  • Replace eq('lastName', 'Simpson') with eq 'lastName', 'Simpson' throughout the spec.
  • Replace like('firstName', 'B%') with like 'firstName', 'B%' in additional criteria closures.

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

jamesfredley added a commit that referenced this pull request May 28, 2026
This commit reverts the portions of this branch that have been split out
into standalone PRs against 8.0.x. The goal is to shrink the PR-A
review surface to focus on the actual hibernate5 -> hibernate7 clone
and let the unrelated cleanups be reviewed on their own merits.

Once PRs B/C/D/E land on 8.0.x and 8.0.x is merged into this branch,
the reverted changes will arrive through the merge so the final state
of stage-hibernate7 is unchanged - only the diff visible on this PR
is reduced.

Reverted content:

  Async defensive cleanup (PR #15682, PR-B)
    9 files in grails-async/. Reverts e3cad41. Null-safety guards,
    @OverRide annotations, catch(Exception ignored) over catch(Throwable),
    constant-on-left equality, varargs.

  addAllDomainClasses helper + adoption (PR #15683, PR-C)
    Helper method on GrailsDataTckManager plus all callers across the
    data mapping test suites. Reverts ed0916d plus all orphan
    callers added by subsequent commits (50 specs) so the branch
    compiles without the helper method.

  MongoDatastoreSpec base class + mongo package rename (PR #15685, PR-E)
    Reverts c8cb43f (MongoDatastoreSpec introduction + ~107 spec
    refactors) and the mongo portion of 01c27f9 (8 file renames
    grails.gorm.tests -> grails.gorm.specs plus 18 import-update
    modifications in non-renamed mongo specs).

  DetachedCriteriaSpec command-chain syntax (PR #15684, PR-D)
    18 `eq(...)` and `like(...)` parens restorations in the TCK
    DetachedCriteriaSpec. Reverts the parens hunk of ee4ab14 while
    leaving the rest of that commit (setupSpec addition, import
    reorder, and the other 35 TCK file changes) intact.

NOT reverted (intentionally kept in PR-A):

  - The hibernate5 -> hibernate7 baseline clone (~100k lines, the
    actual work being reviewed)
  - 89 hibernate5 and 90 hibernate7 grails.gorm.tests -> grails.gorm.specs
    package renames (non-mongo portion of 01c27f9, since these
    touch the cloned tests and reverting would require re-doing both
    sides)
  - 18 grails-datamapping-core-test package renames (non-mongo portion
    of 01c27f9)
  - All other "Pull forward..." commits (styling, build config,
    test additions, etc.) which the reviewers have not specifically
    objected to

Assisted-by: claude-code:claude-4.7-opus
Copy link
Copy Markdown
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

I am against this change and I think we should always use parentheses for method calls as it is much easier to read/skim.

Copy link
Copy Markdown
Contributor

@sbglasius sbglasius left a comment

Choose a reason for hiding this comment

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

I agree with @matrei - removing parentheses does not add value.

@jamesfredley
Copy link
Copy Markdown
Contributor Author

I agree with standardizing on parentheses for method calls. It is the majority, but separate from this we will need to create a issue to go back to cleanup.

on origin/8.0.x (the PR's actual base = historical truth)

Combined eq + like:

Scope paren no-paren split
All .groovy 109 (92 + 17) 97 (77 + 20) ~53 / 47
*Spec.groovy only 94 (79 + 15) 78 (69 + 9) ~55 / 45

Per-method breakdown:

Method / scope paren no-paren
eq - all groovy 92 77
eq - specs only 79 69
like - all groovy 17 20
like - specs only 15 9

Staging vs base (specs only)

Branch paren no-paren
8.0.x (base, historical truth) 94 78
8.0.x-stage-hibernate7 (staging) 95 88

The staging branch added ~10 no-paren spec lines on top of base. The migration toward no-paren is something the hibernate7 staging work itself introduced, not a pre-existing house style.

Verdict

  • Historically on 8.0.x, the parenthesized form is the plurality - roughly 55/45 in specs, 53/47 overall.
  • eq(...) specifically leads in every slice (92 vs 77 overall, 79 vs 69 in specs).
  • The only place no-paren wins is like across all groovy (20 vs 17), and that flips back to paren when restricted to specs (15 vs 9).
  • The HibernateCriteriaBuilder javadoc examples and most example/integration-test apps use the parenthesized form.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants