Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configurable method name prefixes to builders #86

Merged
merged 1 commit into from Feb 2, 2022

Conversation

mads-b
Copy link
Contributor

@mads-b mads-b commented Jan 27, 2022

Sometimes I feel old and conservative and want to do things the old way.

Just kidding;
We have a giant codebase built on top of Immutables.org. It makes builders with setter methods. To make our migration to records as painless as possible, it is nice to migrate without having to rename the usage of Immutables builders for thousands of classes.

Also, get* set* and is* have been idiomatic Java since "forever". Some people might prefer these prefixes, even if they do make the methods three characters longer

@Randgalt
Copy link
Owner

Thanks for the submission - interesting idea to make this a more useful transition from Immutables. What about this - when these options are active create an additional interface (like With) something like this (for an example Person record):

public interface Bean extends With {
    default String getName() {
        return name();
    }

   default int getAge() {
        return age();
  }
}

This way, you can add Bean to your record and it becomes old-school compatible. WDYT?

@Randgalt Randgalt added enhancement New feature or request needs discussion labels Jan 28, 2022
@mads-b
Copy link
Contributor Author

mads-b commented Jan 28, 2022

That's actually quite clever. I like it.

I would, however, hide it behind some flag.
My rationale:
We actually had to disable tons of immutables functionality because immutables generated classes that hit java lang limits on size for types with very large amounts of fields. This especially occurs when feature like the "With" generation is enabled. Class size quite rapidly balloons as a function of field count. Personally, I feel any feature really should be disableable, resulting in the minimum functioning builder in the basic case. Thoughts on this?

Lastly:
Want me to add this Bean feature to the PR, or are you planning on doing it?

@Randgalt
Copy link
Owner

Want me to add this Bean feature to the PR, or are you planning on doing it?

Up to you - let me know.

@mads-b
Copy link
Contributor Author

mads-b commented Jan 28, 2022

Up to you - let me know.

I will take a stab at it in approx 14 hours :-)

@mads-b
Copy link
Contributor Author

mads-b commented Jan 29, 2022

Just posting to let you know I took a stab at it and did not arrive at a result I was happy with. Here's why:

  • I didn't want to extend With, because that meant a user would have to use the With feature; Not ideal. Also, there's methods in With that have no business being available to an instance that is supposed to be read-only.
  • Made an interface named Bean. Sure, I can make default getters, but I have no way of accessing the record, because Bean is the subclass, not the superclass. Sure, I can use composition and make the record a member of Bean, but interfaces cannot have members and records cannot extend classes.

The end of this train of thought would be to introduce a whole new class; So you would possibly have a new build method that returned this bean type rather than the original record. The issue here is that now suddenly all your calling code needs to dereference this generated bean type which merely wraps the original record. I don't really like the thought of not only having to use generated types when building records, but also when using them.. The record sort of ends up never actually being used for anything in the codebase apart from being the metadata input to this library.

Even Sonar thinks this approach is dodgy: https://rules.sonarsource.com/java/RSPEC-6211

So in closing, I'm not going to try to make something I cannot personally recommend the use of, so I suppose it's up to you to figure out something super-clever :)

@Randgalt
Copy link
Owner

It can be a separate interface just like With - it doesn't needs to extend With. So:

public interface Bean {
   String name();   // matches the Record method so being a subclass doesn't matter

   int age();          // matches the Record method so being a subclass doesn't matter

    default String getName() {
        return name();
    }

   default int getAge() {
        return age();
  }
}

@mads-b
Copy link
Contributor Author

mads-b commented Jan 29, 2022

Haha, how did I miss that. Alright, hold on for an hour or so :)

Add configurable Bean interface to add prefixed getters to record
@@ -72,6 +73,9 @@
.addTypeVariables(typeVariables);
addVisibility(recordActualPackage.equals(packageName), record.getModifiers());
addWithNestedClass();
if (!metaData.beanClassName().isEmpty()) {
Copy link
Owner

@Randgalt Randgalt Jan 31, 2022

Choose a reason for hiding this comment

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

This should also check that the other required options are set. I did a quick test where I removed setterPrefix = "set", getterPrefix = "get", booleanPrefix = "is", and you then get compilation failure. Let's either just ignore this setting if the others aren't sent or output a message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? I did the same in CustomMethodNames.class, and it built just fine with an empty bean interface:
/** * Add getters to {@code CustomMethodNames} */ @Generated("io.soabase.recordbuilder.core.RecordBuilder") public interface Bean { }

Copy link
Owner

Choose a reason for hiding this comment

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

I used this:

@RecordBuilder
@RecordBuilder.Options(beanClassName = "Bean")
public record CustomMethodNames(
    int theValue,
    List<Integer> theList,
    boolean theBoolean) implements Bean {
}

I then get:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:testCompile (default-testCompile) on project record-builder-test: Compilation failure: Compilation failure: 
[ERROR] /Users/jordanzimmerman/dev/oss/soabase/record-builder/record-builder-test/src/test/java/io/soabase/recordbuilder/test/TestCustomMethodNames.java:[29,9] cannot find symbol
[ERROR]   symbol:   method setTheValue(int)
[ERROR]   location: class io.soabase.recordbuilder.test.CustomMethodNamesBuilder
[ERROR] /Users/jordanzimmerman/dev/oss/soabase/record-builder/record-builder-test/src/test/java/io/soabase/recordbuilder/test/TestCustomMethodNames.java:[41,13] cannot find symbol
[ERROR]   symbol:   method setTheValue(int)
[ERROR]   location: class io.soabase.recordbuilder.test.CustomMethodNamesBuilder
[ERROR] /Users/jordanzimmerman/dev/oss/soabase/record-builder/record-builder-test/src/test/java/io/soabase/recordbuilder/test/TestCustomMethodNames.java:[53,24] cannot find symbol
[ERROR]   symbol:   method getTheValue()
[ERROR]   location: variable obj of type io.soabase.recordbuilder.test.CustomMethodNames
[ERROR] /Users/jordanzimmerman/dev/oss/soabase/record-builder/record-builder-test/src/test/java/io/soabase/recordbuilder/test/TestCustomMethodNames.java:[54,33] cannot find symbol
[ERROR]   symbol:   method getTheList()
[ERROR]   location: variable obj of type io.soabase.recordbuilder.test.CustomMethodNames
[ERROR] /Users/jordanzimmerman/dev/oss/soabase/record-builder/record-builder-test/src/test/java/io/soabase/recordbuilder/test/TestCustomMethodNames.java:[55,19] cannot find symbol
[ERROR]   symbol:   method isTheBoolean()
[ERROR]   location: variable obj of type io.soabase.recordbuilder.test.CustomMethodNames
[ERROR] -> [Help 1]
[ERROR] 
[ERR

Copy link
Owner

Choose a reason for hiding this comment

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

Oh wait - those errors are from the test - never mind

Copy link
Owner

Choose a reason for hiding this comment

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

However, you do end up with an empty Bean class. Do we want that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, while completely useless, you did enable the feature to generate the interface, so I would claim it's expected behavior. I can of course add a clause to the if to make it go away in this case, but there's still two corner cases that would result in the same empty interface:

Only getter prefix is enabled, but only boolean fields in record
Only boolean prefix is enabled, but no booleans in record

These two would require me to scan the record for these fields, and at that point I feel I'm making a non-negligible amount of code to check for very little gain. Let me know if you still want me to amend that if and just ignore these corner cases

Copy link
Owner

Choose a reason for hiding this comment

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

Well, while completely useless, you did enable the feature to generate the interface

Sure - that makes sense.

@Randgalt Randgalt added this to the v32 milestone Jan 31, 2022
@Randgalt Randgalt merged commit 43bc65e into Randgalt:master Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants