Does toSerializable need to be a single method? Can't we have separate methods that return different things? One method that returns a String, and a different method that returns a LogEvent copy? That would simplify things a lot, I think.
On Wed, Aug 14, 2013 at 11:37 PM, Gary Gregory <[email protected]>wrote: > On Wed, Aug 14, 2013 at 9:52 AM, Remko Popma <[email protected]>wrote: > >> Gary, >> >> The change only affects Appender, Layout is still generic. >> Appender does not need to be generic (it can just return a Layout<? >> extends Serializable> from its getLayout() method). >> >> This change does not introduce any type casts (afaics). >> > > This might be academic, but it would have to introduce type casts since we > have two kinds of layouts: String and LogEvent. > > For example: > > LogEvent logEvent = null; > logEvent = new SerializedLayout().toSerializable(logEvent); > String xmlFragment = XMLLayout.createLayout(null, null, null, > null, null, null).toSerializable(logEvent); > > Gary > > > >> Remko >> >> >> >> On Wed, Aug 14, 2013 at 10:11 PM, Gary Gregory <[email protected]>wrote: >> >>> On Wed, Aug 14, 2013 at 4:32 AM, Remko Popma <[email protected]>wrote: >>> >>>> Patch looks nice. Much fewer "raw type" compiler warnings. Seems like a >>>> big improvement to me. >>>> >>>> A small improvement on the patch would be to remove the (now >>>> unnecessary) @param <T> javadoc comments. I have that additional change >>>> done in my local workspace. >>>> >>>> I wouldn't mind committing this but I don't want to disrupt anyone's >>>> work-in-progress: the patch modifies about 125 files. >>>> >>>> Is everyone ok with me committing this? I'll hold off for a day or two >>>> (or less if we're all ok with this). >>>> >>> >>> Hi All: >>> >>> Please also consider these thought paths: >>> >>> - The raw compiler warnings can also be considered as a 'task not >>> finished' warning. That is, I did not go all the way in pushing generics >>> through the whole code base. >>> >>> - Consider the Appender/Layout generics separately from the other >>> generics in the code. I think this is already the case based on other >>> messages I've read, but I wanted to make sure 'the baby does not get thrown >>> out with the bath water' (a goofy saying here in the US). >>> >>> - Weigh was is 'correct' vs. what is 'convenient' vs. compiler warnings. >>> >>> - What is the goal? For me, the goal of the generics here are multiple: >>> -- Have the ability to write code that, for example, can configure and >>> start Log4j and then manipulate it without type casts. >>> -- Avoid type casts in general. >>> -- Cause compile time errors instead of a runtime errors when >>> extending Log4j. >>> >>> Gary >>> >>> >>>> >>>> Remko >>>> >>>> >>>> On Monday, August 12, 2013, Ralph Goers wrote: >>>> >>>>> I've briefly glanced at your patch and it looks reasonable to me. It >>>>> leaves the Layout to continue to use generics but removes the generics >>>>> from >>>>> the Appenders. That seems like a reasonable thing to do. >>>>> >>>>> Ralph >>>>> >>>>> On Aug 11, 2013, at 6:34 PM, Henning Schmiedehausen wrote: >>>>> >>>>> Ok, that makes a lot of sense. I have reworked the patch to leave >>>>> these alone and put it on a different git branch. I also opened a ticket: >>>>> https://issues.apache.org/jira/browse/LOG4J2-343 and attached it as a >>>>> patch so that if you do not want to deal with git, you can get it from >>>>> there. >>>>> >>>>> I added instructions on how to get the patch from git to the ticket. >>>>> >>>>> >>>>> On Sat, Aug 10, 2013 at 4:40 PM, Nick Williams < >>>>> [email protected]> wrote: >>>>> >>>>>> >>>>>> On Aug 10, 2013, at 6:31 PM, Henning Schmiedehausen wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> I was toying with the log4j 2 API for a new project and I stumbled >>>>>> over the fact that it uses a generic for Appender<T> without actually >>>>>> being >>>>>> generic. The only generic part is the Layout. So as a result there is >>>>>> this >>>>>> weird construct of Appender<SomeSerializableType> which is actually >>>>>> dictated by the layout in use. >>>>>> >>>>>> >>>>>> I'm relatively new to the team, so I don't know much about the >>>>>> reasoning behind making Appender generic, so I can't speak to that. I'm >>>>>> not >>>>>> personally opposed to removing these generics, but that is a HUGE change. >>>>>> >>>>>> This leads to really interesting constructs such as >>>>>> >>>>>> public abstract class AbstractDatabaseAppender<T extends >>>>>> AbstractDatabaseManager> extends AbstractAppender<LogEvent> >>>>>> >>>>>> >>>>>> Well this is a very different case. The <LogEvent> here is about >>>>>> Layout, just as you said. The <T extends AbstractDatabaseManager> is >>>>>> completely unrelated to Layout and I am _not_ in favor of removing these >>>>>> generics. >>>>>> >>>>>> I was wondering whether this is necessary as it makes the API very >>>>>> cumbersome to use and read so I removed the generic from Appender and >>>>>> subsequently went through the log4j 2 code base and mostly removed stuff >>>>>> that was no longer needed once that was gone. The result is at >>>>>> >>>>>> https://github.com/apache/logging-log4j2/pull/1 >>>>>> >>>>>> >>>>>> The Apache GitHub repository is just a mirror of our SVN repository. >>>>>> We can't accept or use any pull requests there. You need to generate an >>>>>> SVN >>>>>> patch and attach it to whatever JIRA you create. (As such, you should >>>>>> close >>>>>> this pull request.) >>>>>> >>>>>> I will also file a JIRA for this. >>>>>> >>>>>> I know that the 2.0 release should be coming soon (being at beta8), >>>>>> but I feel that making that change in the API before it is set in stone >>>>>> with 2.0 woulc be really beneficial for anyone who wants to port code to >>>>>> 2.0 / write new code. >>>>>> >>>>>> >>>>>> I'm sure there will be plenty of discussion about this over the next >>>>>> few days. >>>>>> >>>>>> Thanks for considering, >>>>>> Henning >>>>>> >>>>>> >>>>>> Nick >>>>>> >>>>> >>>>> >>>>> >>> >>> >>> -- >>> E-Mail: [email protected] | [email protected] >>> Java Persistence with Hibernate, Second >>> Edition<http://www.manning.com/bauer3/> >>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >>> Spring Batch in Action <http://www.manning.com/templier/> >>> 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<http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > Blog: http://garygregory.wordpress.com > Home: http://garygregory.com/ > Tweet! http://twitter.com/GaryGregory >
