Repository navigation
Add update helpers xyzUpdate(UnaryOperator<T>) (#223) - #31
devin-ai-integration[bot] wants to merge 8 commits into
Conversation
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
…rs#223) Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
|
Please have a deep thought of naming. Starting all with map, is this intuitive? We would have for field example all functions be named example, and then a new function with mapExample? And is it a mapping? It is more an inside applying!? So please think of these questions! And check other solutions, how this is done there. Maybe having a different naming or should we remain with map? |
| "[DEBUG] Starting BuilderProcessor...", | ||
| "[DEBUG] Loaded global configuration from compiler arguments: BuilderConfiguration[]", | ||
| "[DEBUG] Loaded global configuration from compiler arguments: " | ||
| + "BuilderConfiguration[generateMapperHelpers=DISABLED]", |
There was a problem hiding this comment.
Why do we have this extra logging here? Does it log extra or did you change the design of this test and it needs to be activated and the asserts changed?
There was a problem hiding this comment.
This only changed because the test had disabled the mapper option; that option is removed again in 1938991 and the assertion is back to BuilderConfiguration[].
| * The field must already be set; otherwise the generated method throws {@link | ||
| * IllegalStateException}. |
There was a problem hiding this comment.
This should be in throws field of JavaDoc.
There was a problem hiding this comment.
Removed the sentence from the option Javadoc (1938991) — the unset-case is documented once, in the generated method's @throws IllegalStateException.
| * value must have been set before (directly or via an existing instance). A <code>null</code> result is stored as-is | ||
| * and validated by <code>build()</code> like any other value. |
There was a problem hiding this comment.
Thus topic regarding null might not be in javadoc of all methods. Especially because it is in @throws explained
There was a problem hiding this comment.
Shortened (1938991): the null sentence is gone from the generated Javadoc (the behaviour is the same as for every other setter, so it is documented once in CONFIGURATION.md only); the unset requirement stays in the @throws.
| "Transforms the current value of <code>%s</code> in place by applying the given " | ||
| + "operator, instead of reading it out, changing it and setting it again.\n" | ||
| + "Useful for adjustments relative to the current value, e.g. trimming, " | ||
| + "upper-casing, clamping or incrementing, and in combination with the " | ||
| + "<code>With</code> copy-and-modify flow.\n" | ||
| + "The value must have been set before (directly or via an existing instance).\n" | ||
| + "A <code>null</code> result is stored as-is and validated by <code>build()</code> " | ||
| + "like any other value.", |
| "if <code>%s</code> has not been set yet".formatted(originalFieldName))); | ||
|
|
||
| String mapperExample = | ||
| JavadocExampleValues.getMapperExample(field.getFieldType()).orElse("value -> value"); |
There was a problem hiding this comment.
Does it make sense to have this in examples class. Could that really be reused? Or should we have the mapping and seamless not humere in the generator?
There was a problem hiding this comment.
Agreed, it was not reusable. The selection now lives in MapperHelperGenerator as a private getMapperExample(TypeName); JavadocExampleValues keeps only the generic example values it had before (1938991).
| @SimpleBuilder(options = @SimpleBuilder.Options(generateJavaDoc = OptionState.DISABLED)) | ||
| @SimpleBuilder(options = @SimpleBuilder.Options( | ||
| generateJavaDoc = OptionState.DISABLED, | ||
| generateMapperHelpers = OptionState.DISABLED)) |
There was a problem hiding this comment.
Removed (1938991); the test only disables generateJavaDoc again.
| void configurationMerge_Chain_ShouldApplyInOrder() { | ||
| // Layer 1: Defaults | ||
| BuilderConfiguration defaults = BuilderConfiguration.DEFAULT; | ||
| assertEquals(OptionState.ENABLED, defaults.generateMapperHelpers()); |
There was a problem hiding this comment.
Why do we have here an new assert just for that? This is done different from the others right?
| .withOptions("-Asimplebuilder.formattingMode=lightweight") | ||
| .withOptions( | ||
| "-Asimplebuilder.formattingMode=lightweight", | ||
| "-Asimplebuilder.generateMapperHelpers=DISABLED") |
There was a problem hiding this comment.
Removed; expected outputs in FormattingModeTest include the mapper methods now (1938991).
| Compilation compilation = ProcessorTestUtils.createCompiler().compile(sourceFile); | ||
| Compilation compilation = | ||
| ProcessorTestUtils.createCompiler() | ||
| .withOptions("-Asimplebuilder.generateMapperHelpers=DISABLED") |
| Compilation compilation = createCompiler().compile(sources); | ||
| Compilation compilation = | ||
| createCompiler() | ||
| .withOptions("-Asimplebuilder.generateMapperHelpers=DISABLED") |
There was a problem hiding this comment.
Removed; NullConstraintTest runs with mapper helpers enabled (1938991).
I looked at how others do it before answering. Why not an overload What do others use?
Is it a mapping or "inside applying"? The field holds IDE grouping: agreed, Recommendation: keep |
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
|
Renamed per decision: generated method is now |
Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
| * .name(() -> "example value") | ||
| * .nameUpdate(String::trim) | ||
| * .name(sb -> sb.append("text")) | ||
| * .sponsorenUpdate(UnaryOperator.identity()) |
There was a problem hiding this comment.
This is wrong, right? A call like this is not possible!?
There was a problem hiding this comment.
Correct — the call compiles, but at that position in the chain it would throw IllegalStateException because sponsoren hadn't been set yet (the class-level example is emitted in generator-priority order, and UpdateHelperGenerator sat at 59, above the consumer generators). Fixed in 1e1ce3a by lowering the generator priority to 20 (below every setter variant), so xUpdate(...) now always appears after all setters of that field: .sponsoren(t -> ...) then .sponsorenUpdate(UnaryOperator.identity()). Example builders regenerated, test expectations updated.
There was a problem hiding this comment.
But should it not be TargetType::identity instead?
| * .pagesUpdate(Math::abs) | ||
| * .tags(List.of("example value")) | ||
| * .tags(() -> List.of("example value")) | ||
| * .tagsUpdate(UnaryOperator.identity()) |
There was a problem hiding this comment.
Does it work with List? Or are lists not modifiable in inner tracked code?
There was a problem hiding this comment.
Yes, it works with List (and any other type): the helper hands the operator the exact reference currently stored in the TrackedValue and stores whatever the operator returns — the builder doesn't wrap or copy collections. So the operator can return a new list (l -> { var c = new ArrayList<>(l); c.add("b"); return c; }), or mutate and return the same instance if the stored list is mutable (as it is after the tags(t -> t.add(...)) ArrayListBuilder path; a List.of(...) passed via the plain setter is immutable, so there a new list must be returned). Added two runtime tests in 1e1ce3a covering tagsUpdate after the direct setter and after the consumer path.
There was a problem hiding this comment.
But is not the listbuilder producing unmodifiable lists? And how is the setter of sets and collections working? Is it taking over a list from external? Should it then not be a new list which takes the elements? How does that behave by with-Interface? Would a change there in list of copy of object, lead to a change of the list of origin? Thinking of that, this might be even the general behavior when having dtos there in properties!? Maybe this is then a totally different issue to handle this!?
|
moved to java-helpers#283 |
Summary
Implements java-helpers#223: a
generateUpdateHelpersoption (defaultENABLED;DISABLEDin@SimpleMinimalBuilder) that adds one transform method per field:UpdateHelperGenerator(priority 59, registered via theGeneratorservice file) so it participates in the registry anddeactivateGenerationComponents=UpdateHelperGeneratorfiltering. Applies to every field; primitives get boxed operator types (UnaryOperator<Integer>) exactly like the existingSupplier<T>setters.Update(MethodGeneratorUtil.generateBuilderMethodName(field) + "Update"), sosetterSuffix=withyieldswithName(...)/withNameUpdate(...). The suffix form was chosen overmapX/updateXso the helper sorts next to its setter in IDE completion; an overloadname(UnaryOperator)is not possible because implicitly typed lambdas would be ambiguous with the existingConsumeroverloads.IllegalStateException(fail-fast as decided in the issue). Values copied by theWithinterface areinitialValue(...)and therefore count as set, soinstance.with(b -> b.nameUpdate(String::toUpperCase))works.nullresults: the helper stores the operator's result as-is, exactly likeSuppliersetters;build()'s existing non-null validation then rejectsnullfor required (non-nullable/primitive) fields with"Field '…' is marked as non-null but null value was provided", while nullable fields accept it. No new generator code — covered by tests and documented in CONFIGURATION.md.@throws IllegalStateExceptionfor the unset case. Examples are chosen inUpdateHelperGenerator.getUpdateExample(TypeName):String::trimfor strings,Math::absfor numeric primitives/wrappers,UnaryOperator.identity()otherwise.PRIORITY_LOW, so for a DTO with fieldsString testandUnaryOperator<String> testUpdatethe existingBuilderDefinitionCreator.resolveMethodConflictskeeps the plaintestUpdate(UnaryOperator<String>)setter, drops the helper fortestand logs a "Method conflict resolved" warning;testUpdateUpdate(...)for thetestUpdatefield is still generated. Different types (String testUpdate) produce both overloads.generateStringFormatHelpers:CompilerArgumentsEnum,SimpleBuilder.Options,BuilderConfiguration,BuilderConfigurationReader,CompilerArgumentsReader.docs/CONFIGURATION.md(Helper Methods section, minimal template, options list, complete example),docs/CUSTOMIZING.mdgenerator table.CustomerDtoBuilder(minimal builder) is unchanged. Existing exact-output tests run with the option enabled; their expected outputs include the update methods.Tests (
UpdateHelperGeneratorTest, 16 tests): on by default,-A/annotation forms incl. DISABLED, transform on set values (" bob "→"BOB",10→20), throw on unset,Withcopy-and-modify, component deactivation, both collision scenarios, null result on@NotNullString / primitiveint(build() throws, helper call itself doesn't) and on a nullable field (builds with null).BuilderProcessorTestgenerator count 14 → 15. Full processor suite: 435 tests green.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