Yes I see it now. I’ve got to say that the code now sure looks a lot more complicated then before you started cleaning it up. You don’t happen to work for the IRS on tax code simplification do you? ;-)
Seriously, i don’t mind separating the plugin building to a separate class, but the builders and visitors make it harder to figure out what exactly is going on. I can see that it may be making it easier to add flexibility, but it may be flexibility that we won’t want in the end. For example, i don’t think we should support both plugin builders and factories - we should only do it one way. However, I appreciate that right now you are trying to sell the idea of using builders instead of factories. To that I’m afraid I don’t understand why createLayout returning newBuildedr() is somehow better than returning new PatternLayout(). In fact, I don’t understand why the @pluginFactory annotation is still in PatternLayout if @PluginBuilderFactory takes precedence. I guess after looking at how the factory method works and the builder works I would say that I really don’t care which way we do it, but not both. That is confusing as it requires the user manual to document both and will require users to be familiar with both syntaxes. I really don’t think one way has any real advantage over the other. They are just different. Is it really worth the effort of changing all the plugins to do this? Ralph On Jun 1, 2014, at 5:55 PM, Matt Sicker <[email protected]> wrote: > It's mostly concentrated in the PluginBuilder class. And in PluginBuilder, > the strategy used for each annotation is determined by that annotation's > annotation which specifies a PluginVisitor class. > > A plugin class is still scanned for a static method annotated with > @PluginFactory, but it's also scanned for @PluginBuilderFactory. If the > builder version is available, that method is tried first for creating the > plugin object. > > I'll update the javadocs in AbstractConfiguration. I notice now that I never > updated them in the first place, so I can see the confusion! > > > On 1 June 2014 19:28, Ralph Goers <[email protected]> wrote: > I have to admit I no longer have a clue how the plugins are created. The > javadoc in AbstractConfiguration still talks about the original annotations > but i’m not really sure where the code that constructs the factory or builder > parameter lists moved to. can you provide a little help? > > Ralph > > On Jun 1, 2014, at 4:13 PM, Matt Sicker <[email protected]> wrote: > >> Alright, I've committed what I was talking about before in regard to builder >> classes and fields. The new @PluginBuilderAttribute annotation could be >> extended to allow using a setter method as well, but I think it's fine for >> now to just use fields. >> >> Anyway, using this idea, there's no longer a need to work around the >> limitations of annotations for specifying default values. >> >> >> On 1 June 2014 10:11, Gary Gregory <[email protected]> wrote: >> On Sat, May 31, 2014 at 9:48 PM, Ralph Goers <[email protected]> wrote: >> 1. That isn't consistent. >> 2. If someone wanted to set the default value of a Log4j component via a >> system property it wouldn't work. That said, I'm not sure how great a >> benefit that actually is. >> >> To be honest, I think I actually would prefer >> >> @PluginAttribute("foo") @IntParameterDefault(1) int foo >> >> It just seems forced to have multiple plugin attribute types just so the >> default value is strongly typed. >> >> Yes, I think this is the change I and others have been proposing. >> >> Gary >> >> >> Ralph >> >> >> On May 31, 2014, at 5:56 PM, Gary Gregory <[email protected]> wrote: >> >>> Well if you do not want strong typing, use a String for foo. >>> >>> Right? >>> >>> Gary >>> >>> >>> -------- Original message -------- >>> From: Ralph Goers >>> Date:05/31/2014 20:38 (GMT-05:00) >>> To: Log4J Developers List >>> Subject: Re: PluginAttribute#defaultEnum()? >>> >>> Gary, I know you said you didn't like >>> >>> @PluginAttribute(name="foo", defaultValue="1") int foo >>> >>> But after looking at >>> http://maven.apache.org/plugin-tools/maven-plugin-annotations/apidocs/org/apache/maven/plugins/annotations/Parameter.html >>> I am wondering if that isn't the best way to go. Doing that would allow >>> the default value to be something like ${sys:fooDefault} which you can't do >>> with strong typing. >>> >>> Ralph >>> >>> Sent from my iPad >>> >>> On May 31, 2014, at 4:25 PM, Ralph Goers <[email protected]> wrote: >>> >>>> Why wouldn't you do >>>> >>>> @PluginAttributeInt(value="foo", defaultValue=1) int foo >>>> >>>> ? >>>> >>>> That said, I'm really not crazy about this. I wish you could do >>>> >>>> @PluginAttribute<Integer>(value="foo", defaultValue=1) int foo >>>> >>>> Ralph >>>> >>>> Sent from my iPad >>>> >>>> On May 31, 2014, at 12:50 PM, Gary Gregory <[email protected]> wrote: >>>> >>>>> So the bummer is that this redundant with the arg type in the signature: >>>>> >>>>> @PluginAttributeInt (value="foo", defaultInt=1) int foo >>>>> >>>>> We should also pick up the attribute name by reflection if it is missing >>>>> from the annotation. >>>>> >>>>> But you'd only need it to spec a default value. So could we keep >>>>> PluginAttribute with only a value and then have an annotation for each >>>>> default value type? @DefaultInt and so on..? >>>>> >>>>> Ideally: >>>>> >>>>> @PluginAttribute() @DefaultInt (1) int foo >>>>> >>>>> Thoughts? >>>>> >>>>> Gary >>>>> >>>>> >>>>> -------- Original message -------- >>>>> From: Paul Benedict >>>>> Date:05/30/2014 22:15 (GMT-05:00) >>>>> To: Apache Log4J Developers >>>>> Subject: Re: PluginAttribute#defaultEnum()? >>>>> >>>>> Totally cleaner. >>>>> >>>>> On May 30, 2014 8:58 PM, "Gary Gregory" <[email protected]> wrote: >>>>> On Fri, May 30, 2014 at 9:39 PM, Paul Benedict <[email protected]> >>>>> wrote: >>>>> I think you guys are better off doing separate annotations to do strong >>>>> typing. You could use a stereotyping pattern like Bean Validation spec. >>>>> >>>>> public @interface PluginAttribute ... >>>>> >>>>> Then annotate other annotations with that: >>>>> @PluginAttribute >>>>> public @interface IntPluginAttribute >>>>> >>>>> >>>>> I did propose that earlier ;-) It seems much cleaner... >>>>> >>>>> Gary >>>>> >>>>> See how the Been Validation spec works to copy their pattern. >>>>> >>>>> On May 30, 2014 7:55 PM, "Gary Gregory" <[email protected]> wrote: >>>>> Hm... you cannot use Enum in the return type for an annotation attribute, >>>>> only an actual enum, like RetentionPolicy. So it seems a no-go. >>>>> >>>>> Gary >>>>> >>>>> >>>>> On Fri, May 30, 2014 at 4:24 PM, Matt Sicker <[email protected]> wrote: >>>>> I had the same hold-up when thinking about adding that, too. How about >>>>> RetentionPolicy.SOURCE? Or ElementType.TYPE? Something annotation-related >>>>> like that. >>>>> >>>>> >>>>> On 30 May 2014 13:12, Gary Gregory <[email protected]> wrote: >>>>> I added PluginAttribute#defaultClass() since we can have a Class has an >>>>> attribute value. >>>>> >>>>> One can also have an Enum as an attribute value, but what default should >>>>> be use? >>>>> >>>>> Gary >>>>> >>>>> -- >>>>> E-Mail: [email protected] | [email protected] >>>>> Java Persistence with Hibernate, Second Edition >>>>> JUnit in Action, Second Edition >>>>> Spring Batch in Action >>>>> Blog: http://garygregory.wordpress.com >>>>> Home: http://garygregory.com/ >>>>> Tweet! http://twitter.com/GaryGregory >>>>> >>>>> >>>>> >>>>> -- >>>>> Matt Sicker <[email protected]> >>>>> >>>>> >>>>> >>>>> -- >>>>> E-Mail: [email protected] | [email protected] >>>>> Java Persistence with Hibernate, Second Edition >>>>> JUnit in Action, Second Edition >>>>> Spring Batch in Action >>>>> Blog: http://garygregory.wordpress.com >>>>> Home: http://garygregory.com/ >>>>> Tweet! http://twitter.com/GaryGregory >>>>> >>>>> >>>>> >>>>> -- >>>>> E-Mail: [email protected] | [email protected] >>>>> Java Persistence with Hibernate, Second Edition >>>>> JUnit in Action, Second Edition >>>>> Spring Batch in Action >>>>> Blog: http://garygregory.wordpress.com >>>>> Home: http://garygregory.com/ >>>>> Tweet! http://twitter.com/GaryGregory >> >> >> >> >> -- >> E-Mail: [email protected] | [email protected] >> Java Persistence with Hibernate, Second Edition >> JUnit in Action, Second Edition >> Spring Batch in Action >> Blog: http://garygregory.wordpress.com >> Home: http://garygregory.com/ >> Tweet! http://twitter.com/GaryGregory >> >> >> >> -- >> Matt Sicker <[email protected]> > > > > > -- > Matt Sicker <[email protected]>
