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

Allow a custom method name prefix #78

Closed
wants to merge 2 commits into from
Closed

Conversation

Vad1mo
Copy link

@Vad1mo Vad1mo commented Sep 23, 2014

This contribution allows to provide a custom method name prefix instead of "with"

IMHO "with" is obsolete, as it is clear when using a builder. Also it is too much redundancy.

How to set a custom prefix or remove it.

There are two ways.

  1. Create and add a pojobuilder.properties file to your classpath.
    The property file should contains the key net.karneim.pojobuilder.methodnameprefix=xx
  2. Annotation Processor Arguments (-A parameter)
    http://docs.oracle.com/javase/7/docs/technotes/guides/apt/GettingStarted.html
    add argument -Anet.karneim.pojobuilder.methodnameprefix=xx to annotation processor.

Maven Example:

         <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-compiler-plugin</artifactId>
            <configuration>
               <compilerArgument>-Anet.karneim.pojobuilder.methodnameprefix=zzz</compilerArgument>
               <!-- alternative for no prefix -->
               <compilerArgument>-Anet.karneim.pojobuilder.methodnameprefix</compilerArgument>
            </configuration>
         </plugin>

Depending on the eclipse and m2e annotation processor setup this setting will be also used by eclipse.

@drekbour
Copy link
Contributor

withX is certainly a choice the library should allow (I like it personally but totally support that it may be redundant or perhaps you prefer avecX or mitX :)). I don't think proc arguments are the way to implement this though! Config is done per-builder via the @GeneratePojoBuilder annotation.

@Vad1mo
Copy link
Author

Vad1mo commented Sep 24, 2014

I understand your concern as it is not the way the library is currently configured.
This is indeed not the best approach, but one that works and allows a global pojobuilder configuration.

Let me explain on this example, why I think a global configuration is a good idea.

@XmlAccessorType(XmlAccessType.FIELD)
@XmlType(name = "TradePriceType", propOrder = { "chargeAmount", "basis", "allowanceCharges" })
@GeneratePojoBuilder(withBuilderInterface = Builder.class, withBuilderProperties = true, methodprefix="with")
public class Price {

@Valid
@NotNull(groups = Comfort.class)
@XmlElement(name = "ChargeAmount")
@XmlJavaTypeAdapter(value = AmountRoundingAdapter.class)
@Basic
private Amount chargeAmount;

I have four different libraries on my little class jaxb, pojobuilder, validation and my own.

There are around 80 such classes. I want to keep it a DRY and maintain it all in one place.

My dream would be to have a configuration annotation

  1. on root package level
  2. free floating on a config class
@PojoBuilderGlobalConfiguration(
    withBuilderInterface = Builder.class, 
    withBuilderProperties = true, 
    methodprefix="avec"
    substituteAnnotationWith={XmlType.class}
)
public class ApplicationConfig {

The Configuration would allow two things.

  1. Sets the method name prefix, and all other parameters.
  2. Substitute default @GeneratePojoBuilder with a different Annotation,
    This allows me to globally configure and make for example all my JaxB classes have a builder.
    This would be also possible on classes not under my controls such as generated jaxb classes.

@drekbour
Copy link
Contributor

I feel your pain :) Are you aware Pojobuilder now supports meta-annotations (https://github.com/mkarneim/pojobuilder#default-configuration-and-meta-annotations). This allows @GeneratePojoBuilder to be placed on a more readable single @VadimBean class annotation.

You can also have a single external class full of static factory methods if that suits you and remove the PB annotation from the pojos themselves:

@GeneratePojoBuilder
public static Price price() { return new Price(); }

Back to the code though. I think you have two excellent items here but have conflated them so they serve a single narrow usecase of both at once:

  1. Add methodPrefix feature to annotation
  2. Make processor defaults configurable via javac -A or "magic file"

@Vad1mo
Copy link
Author

Vad1mo commented Sep 24, 2014

I know about meta-annotations and static factory methods I use them both now.

But still it can't top this global configuration.

@PojoBuilderGlobalConfiguration(
    withBuilderInterface = Builder.class, 
    withBuilderProperties = true, 
    methodprefix="avec"
    substituteAnnotationWith={XmlType.class}
)
public class ApplicationConfig {

What next?

@drekbour
Copy link
Contributor

I really like the idea of setting processor-level defaults via processor config (how can you have a global annotation, what does it sit on, how do you stop there being >1 etc...)

My personal opinion (I'm not the owner of PB) is that you should cancel the PR and submit two enhancements.

@mkarneim
Copy link
Owner

mkarneim commented Oct 1, 2014

I also think these are two features in one.

Right now I am short of time - I will have a closer look at the weekend.

@Vad1mo
Copy link
Author

Vad1mo commented Oct 2, 2014

I am trying to do something like @GeneratePojoBuilder(... withMethodPrefix="myPrefix")

How should I set the prefix in PropertyM?

new PropertyM(propertyName, propertyType, methodNamePrefix);

Should it be there at all?

this forces getOrCreate to provide the parameter and I don't have the prefix in all cases in the Directive.

You can see what I mean here.
https://github.com/Vad1mo/pojobuilder/compare/feature/customPrefix?expand=1

@drekbour
Copy link
Contributor

drekbour commented Oct 2, 2014

I'd say the name of the with method shoudn't be owned by the property model at all (which you clearly suspect as well)

You could replace prop.getWithMethodName with BuilderSourceGenerator.withMethodNameFor(prop) or something similar. I believe the generator should have all of the directives available.

This is going to conflict horribly with PR #79 as we've edited all the same lines of code :)
I suggest you look at the diff for #79 actually as it will give a few pointers for passing the directives through to the generator.

@mkarneim
Copy link
Owner

mkarneim commented Oct 3, 2014

I would copy the name pattern to some central place inside the BuilderM and then pass that pattern to the PropertyM whenever I want to generate the actual method name.

I have implemented this feature into this branch: https://github.com/mkarneim/pojobuilder/tree/feat-settername

If this solution solves the issue "allow a custom method name prefix" for you, I would like to merge it into the master branch and close this issue here.

Concerning the other issue about "global configuration" I would suggest that you open another issue so we can discuss it there.

Comments?

@Vad1mo
Copy link
Author

Vad1mo commented Oct 3, 2014

tried out the feat-settername branch. It Is perfect

@Vad1mo Vad1mo closed this Oct 3, 2014
@mkarneim mkarneim modified the milestone: 3.2.0 Oct 5, 2014
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.

None yet

3 participants