Skip to content

Support external Eclipse formatter profile via -Asimplebuilder.formatterProfile (#278) - #30

Closed
devin-ai-integration[bot] wants to merge 3 commits into
mainfrom
devin/1788610730-formatter-profile-278
Closed

devin-ai-integration[bot] wants to merge 3 commits into
mainfrom
devin/1788610730-formatter-profile-278

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 5, 2026

Copy link
Copy Markdown

Summary

Resolves java-helpers#278: the JDT formatter profile used for generated builders can now be replaced per project instead of being hard-wired to the bundled eclipse-java-format.xml.

New project-wide compiler option (no annotation override, as scoped in the issue):

-Asimplebuilder.formatterProfile=<file path | classpath resource>

Resolution in RoasterSourceFormatter.loadFormatterProperties():

if no custom profile configured      -> loadBundledProfile()
else loadConfiguredProfile(location)  // regular file first, then classpath resource; warns "…falling back to the bundled profile"
       .orElseGet(loadBundledProfile) // warns "…not found on the processor classpath"; empty Properties
                                      //   -> existing "JDT requested but profile unavailable" lightweight fallback

Both are built on a shared readProfile(location); the path is used as given (no placeholder substitution — ${project.basedir} in the Maven example is interpolated by Maven before javac runs).

Plumbing: CompilerArgumentsEnum.FORMATTER_PROFILEProcessingContext.getFormatterProfile() (trimmed, null = default) → RoasterCodeGenerator(ProcessingContext) (now the single constructor; the context gained getProcessingEnvironment()/getLogger()) → RoasterSourceFormatter(logger, mode, formatterProfile) (3-arg ctor now public, blank → default).

Behavior change to note: a missing custom profile previously (via the package-private test ctor) degraded to lightweight formatting; now it falls back to the bundled profile with a warning, so JDT formatting stays on. Two RoasterSourceFormatterTest cases asserting the old lightweight degradation were replaced by constructor_missingProfile_fallsBackToBundledProfile; the "profile unavailable → lightweight" path now only triggers when the bundled resource itself is absent.

Tests: filesystem profile (tab-indent variant of the bundled XML, asserts tabs in output vs spaces for default), explicit classpath name, missing path, blank value, malformed XML, CompilerArgumentsReader read, and an end-to-end compilation with -Asimplebuilder.formatterProfile=<tempfile> in ConfigurationProcessingTest. Documented under formattingMode in docs/CONFIGURATION.md.

Full processor suite: 423 tests green; example module regenerates byte-identically.

Link to Devin session: https://app.devin.ai/sessions/d691231fba0842a4b842b275ca4b35ed
Open in Devin Desktop: https://app.devin.ai/desktop/session/d691231fba0842a4b842b275ca4b35ed?variant=devin
Requested by: @AndreasIgel

devin-ai-integration Bot and others added 2 commits September 5, 2026 12:23
…formatterProfile (java-helpers#278)

Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

Comment thread docs/CONFIGURATION.md
**Maven**:
```xml
<compilerArgs>
<arg>-Asimplebuilder.formatterProfile=${project.basedir}/config/eclipse-formatter.xml</arg>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is ${project.basedir} really supported here? I would have expected, that a path (relative or absolute) would be needed here and the processor would not interpret an environment variable

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right that the processor does no placeholder substitution — it uses the string as given (file path first, then classpath resource). ${project.basedir} works in the example only because Maven interpolates it inside pom.xml before javac is started, so the processor receives an absolute path. Clarified that in the docs in 2e1b3a8 (and that relative paths resolve against javac's working directory).

Comment on lines +96 to +97
new RoasterCodeGenerator(processingEnv, logger, context.getPerformanceTracker());
new RoasterCodeGenerator(
processingEnv, logger, context.getPerformanceTracker(), context.getFormatterProfile());

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Instead of adding a new parameter here, does context contain the processingEnv or code versa? So just giving one of both into constructor?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point — ProcessingContext already holds the ProcessingEnvironment, logger, performance tracker and now the formatter profile. Done in 2e1b3a8: RoasterCodeGenerator has a single constructor RoasterCodeGenerator(ProcessingContext); added getProcessingEnvironment()/getLogger() to the context and updated the resilience test accordingly.

private RoasterSourceFormatter getFormatter(FormattingMode mode) {
return formatterCache.computeIfAbsent(mode, m -> new RoasterSourceFormatter(logger, m));
return formatterCache.computeIfAbsent(
mode, m -> new RoasterSourceFormatter(logger, m, formatterProfile));

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please put the creation of a formatting profile into separate line or onto a helper function

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done in 2e1b3a8: getFormatter now uses computeIfAbsent(mode, this::createFormatter) with a dedicated createFormatter(FormattingMode) helper.

.getResourceAsStream(formatterProfileResource)) {
boolean fallbackToBundled =
!DEFAULT_FORMATTER_PROFILE_RESOURCE.equals(formatterProfileResource);
Optional<Properties> configured = loadProfile(formatterProfileResource, fallbackToBundled);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Having selector booleans in function calls is an antipattern right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed. Done in 2e1b3a8: the boolean is gone — loadFormatterProperties now branches once on whether a custom profile is configured and delegates to loadConfiguredProfile(location) (warns "…falling back to the bundled profile") / loadBundledProfile() (warns "…not found on the processor classpath"), both built on a shared readProfile(location).

Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
@AndreasIgel

Copy link
Copy Markdown
Owner

moved to java-helpers#284

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.

Support external Eclipse formatter profile via compiler argument

1 participant