Randgalt / record-builder

Record builder generator for Java records
Apache License 2.0
756 stars 55 forks source link

Add configurable method name prefixes to builders #86

Closed mads-b closed 2 years ago

mads-b commented 2 years ago

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 commented 2 years ago

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?

mads-b commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Up to you - let me know.

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

mads-b commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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