Skip to content

feat: array/list parameter support for Gestalt-DI annotations#160

Open
BenjaminAmos wants to merge 3 commits intodevelopfrom
temp/terasology-gestalt-di-migration-changes
Open

feat: array/list parameter support for Gestalt-DI annotations#160
BenjaminAmos wants to merge 3 commits intodevelopfrom
temp/terasology-gestalt-di-migration-changes

Conversation

@BenjaminAmos
Copy link
Copy Markdown
Contributor

@BenjaminAmos BenjaminAmos commented Apr 15, 2025

These changes are needed for Terasology to work with Gestalt DI. This is to go along with the main Terasology pull request.

It appears to do the following (although I am no longer certain of this):

  • Add additional input validation to catch null beans when closing the context
  • Upgrade the JavaPoet library (although I am unable to recall why by this point)
  • Fixes for using array/list values as annotation parameters when inspected by Gestalt DI's annotation processor
  • Change !semver.preReleaseVersion().isEmpty() to semver.preReleaseVersion().isPresent() (this may have been due to a breaking ABI change in the JSemver library)

@BenjaminAmos BenjaminAmos changed the title temp: Terasology Gestalt-DI migration changes feat: array/list parameter support for Gestalt-DI annotations Aug 10, 2025
@BenjaminAmos BenjaminAmos force-pushed the temp/terasology-gestalt-di-migration-changes branch from 2d99dbb to b2a6b05 Compare March 25, 2026 11:28
@Cervator
Copy link
Copy Markdown
Member

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 31, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 31, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 72df3859-2132-48bc-b9b6-771a7e91058f

📥 Commits

Reviewing files that changed from the base of the PR and between b2a6b05 and 3626728.

📒 Files selected for processing (1)
  • gestalt-inject-java/src/main/java/org/terasology/gestalt/annotation/processing/BeanDefinitionProcessor.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • gestalt-inject-java/src/main/java/org/terasology/gestalt/annotation/processing/BeanDefinitionProcessor.java

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Prevented recursive closure of the dependency-injection context.
    • Improved annotation processing to use type-aware value formatting (better enum and list handling).
    • Corrected resource path resolution so directory and file lookups are distinguished.
    • Fixed version snapshot detection to use proper presence semantics.
  • Chores

    • Updated Javapoet dependency to a newer patch version.

Walkthrough

Small cross-module changes: prevented a bean context from closing itself, bumped JavaPoet, made annotation-value formatting type-aware, fixed classpath file vs directory path construction, and adjusted snapshot detection to use presence semantics.

Changes

Cohort / File(s) Summary
Dependency Update
gestalt-inject-java/build.gradle.kts
Bumped com.squareup:javapoet from 1.12.0 to 1.13.0.
Bean Context
gestalt-di/.../DefaultBeanContext.java
Added identity check to avoid invoking close() on the context instance itself during cleanup.
Annotation Processing
gestalt-inject-java/.../BeanDefinitionProcessor.java
Reworked annotation-value conversion to getValue(TypeMirror type, Object target) to format lists and enums based on the declared annotation return type.
Classpath Resource Handling
gestalt-module/.../ClasspathFileSource.java
Distinguished directory vs file lookups by adding isDirectory to buildPathString, controlling trailing separator behavior for resource resolution.
Version Logic
gestalt-module/.../Version.java
Changed snapshot detection to use semver.preReleaseVersion().isPresent() instead of !...isEmpty().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

"I’m a rabbit in the code-tree glade,
nibbling bugs that version made.
I hop past paths and type the tune,
keep contexts safe — I humbly prune.
Hooray for tiny, tidy change!" 🐇

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: array/list parameter support for Gestalt-DI annotations' directly matches the main change described in the raw_summary for BeanDefinitionProcessor.java, which implements type-aware formatting for annotation values including list defaults.
Description check ✅ Passed The description is related to the changeset, covering the main changes: null bean validation, JavaPoet upgrade, array/list annotation parameter fixes, and semver API change.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch temp/terasology-gestalt-di-migration-changes

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@gestalt-inject-java/src/main/java/org/terasology/gestalt/annotation/processing/BeanDefinitionProcessor.java`:
- Line 247: In the values.add(...) call that currently passes
executableElement.asType() into getValue(...), replace asType() with
executableElement.getReturnType() so getValue() receives the annotation member's
return type (not the full executable signature); update the invocation near
values.add(CodeBlock.of("$S,$L", executableElement.getSimpleName(),
getValue(executableElement.asType(), value.getValue()))) to use getReturnType()
to fix enum detection and correct empty-array literal generation (consistent
with the earlier usage at line 238).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8c5540a6-e2c1-414e-a2da-b99e7eb25ec1

📥 Commits

Reviewing files that changed from the base of the PR and between 33d0591 and b2a6b05.

📒 Files selected for processing (5)
  • gestalt-di/src/main/java/org/terasology/gestalt/di/DefaultBeanContext.java
  • gestalt-inject-java/build.gradle.kts
  • gestalt-inject-java/src/main/java/org/terasology/gestalt/annotation/processing/BeanDefinitionProcessor.java
  • gestalt-module/src/main/java/org/terasology/gestalt/module/resources/ClasspathFileSource.java
  • gestalt-module/src/main/java/org/terasology/gestalt/naming/Version.java

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.

3 participants