On Wed, May 30, 2012 at 5:29 PM, Thomas Mortagne <[email protected]> wrote: > On Wed, May 30, 2012 at 4:01 PM, Eduard Moraru <[email protected]> wrote: >> On Wed, May 30, 2012 at 4:16 PM, Thomas Mortagne >> <[email protected]>wrote: >> >>> On Wed, May 30, 2012 at 2:55 PM, Eduard Moraru <[email protected]> >>> wrote: >>> > I mentioned Velocity because I saw a DiffScriptService component with >>> > @Named("diff") that has the same method signature as the Java API: >>> > >>> > public <E> DiffResult<E> diff(List<E> previous, List<E> next, >>> > DiffConfiguration<E> configuration) >>> > >>> > See the discussion [1] on the pull request for more details on the >>> Velocity >>> > question. >>> > >>> > Back to my original question (same question as Thomas basically), that's >>> > exactly what I am trying to understand, Vincent... if the >>> > List<String/Character> is the optimal use case and not just String. >>> > >>> > Without a scenario in favour of List<E>, I would go for exposing a String >>> > based API (well, just an extra method in this case) as well. >>> >>> You can imagine all sort of use cases with any value so I don't see >>> the point in arbitrary limiting the API to just Strings where the >>> logic is the same whatever the type. >> >> >> Yes, I understand now how it works. If other are interested, please see my >> new comment [1] on the pull request. >> >> Short version: The API compares lists of objects by using Object#equals() >> and does not go into the structure of the compared elements, just reports >> differences of the list itself. You have to structure/break your input to >> the level of granularity that you need for the diff to be relevant to your >> use case. >> >> >>> If you really need a direct use >>> case for existing stuff then just think about producing a diff between >>> two static list properties values, it would be very bad to have to >>> create a String from it first especially since you could have \n in >>> one of the elements. >>> >>> My main concern is to expose a clear API and no String it everything >>> but clear, it's simple yes but it's not the same things. When you >>> provide String you don't have no idea what you are diffing exactly >>> (could be characters, \n, words, etc.) so having only one method with >>> two strings is just very bad design. >>> >>> Also if you read my mail I did talked about String based APIs but as >>> helpers and I'm would be surely -1 for any attempt to restrict the >>> whole API to it. So the only question here is: is that really needed >>> to expose String based helpers or not. >>> >> >> No, I was never suggesting to drop the generic API, I was just saying that >> the helpers might be a good idea since I imagine that String diff would >> cover most of the usage of this API. Anyway, this is not important for now >> since it is something that can be added later if API clients start >> complaining :) >
> That's more or less what I was planning, I'm waiting for my first > client report now, any news Marius ? ;) Yes. Works great! I rewrote my display-oriented diff builders to use the new API. I have 3 remarks about the code: (1). The third parameter you pass to DiffManager when creating a diff, DiffConfiguration, is a bit awkward. At first I though it was a class. Then I looked for an implementation. Then I looked at the test and saw that you pass null. I'm not convinced the configuration is very useful. At least we could add a new signature without the DiffConfiguration parameter. I'm thinking that we could remove it and use instead a ConfigurationSource for global configuration. (2) I don't understand what getCurrentMergeResult has to do with MergeConfiguration. I haven't done any merge yet, I just saw this method while looking for a diff configuration implementation and it strike me. (3) I think that getUnifiedDiff could be moved outside of DiffResult. I haven't use it since I have my own display-oriented builders (to help me display a diff), but I think it's too specific to be placed inside the DiffResult interface. Thanks, Marius > >> >> Thanks, >> Eduard >> >> ---------- >> [1] https://github.com/xwiki/xwiki-commons/pull/2/files#r900063 >> >> >>> > >>> > Thanks, >>> > Eduard >>> > >>> > ---------- >>> > [1] https://github.com/xwiki/xwiki-commons/pull/2/files#L27R58 >>> > >>> > On Wed, May 30, 2012 at 3:32 PM, Vincent Massol <[email protected]> >>> wrote: >>> > >>> >> >>> >> On May 30, 2012, at 2:28 PM, Vincent Massol wrote: >>> >> >>> >> > >>> >> > On May 30, 2012, at 2:18 PM, Eduard Moraru wrote: >>> >> > >>> >> >> Hi Thomas, >>> >> >> >>> >> >> On Tue, May 29, 2012 at 5:07 PM, Thomas Mortagne >>> >> >> <[email protected]>wrote: >>> >> >> >>> >> >>> Hi devs, >>> >> >>> >>> >> >>> As I said in another mail I'm working on a diff/merge module to use >>> in >>> >> >>> XWiki, the first target being Extension Manager and document history >>> >> >>> (since both are going to use the same code, see Marius mails). >>> >> >>> >>> >> >>> You can see the detail on >>> >> https://github.com/xwiki/xwiki-commons/pull/2. >>> >> >>> >>> >> >>> TODO/QUESTIONS: >>> >> >>> * improve the generic 3 ways merge to be at least as good as the >>> >> >>> List<String> 3 ways merge and get rid of JDiff >>> >> >>> * not sure where/if I should put String related helpers (things that >>> >> >>> do a diff on two String instead of having to cut List<String> or >>> >> >>> List<Character> and call the diff API etc.) >>> >> >>> >>> >> >> >>> >> >> This looks a bit awkward to use, specially from (but not limited to) >>> >> >> Velocity. When I think of a diff service API, I imagine passing >>> Strings, >>> >> >> not lists of composing strings or list of characters. >>> >> > >>> >> > Thomas is proposing a Java API here AFAIK. For Velocity we never >>> expose >>> >> java API directly; we use Script Services for that. >>> >> >>> >> Sorry, just realized you wrote "but not limited to" ;) >>> >> >>> >> I'll let Thomas reply since I don't know this diff api yet…. >>> >> >>> >> Sorry for the noise >>> >> -Vincent >>> >> >>> >> > >>> >> > Thanks >>> >> > -Vincent >>> >> > >>> >> > PS: FTR I'm a big -1 to model our Java APIs to be easily usable from >>> >> Velocity since that means having suboptimal APIs for the wrong reason ;) >>> >> > >>> >> >> Also, I don`t quite understand the List<String> approach. I can >>> imagine >>> >> the >>> >> >> List<Character> version by breaking a string into characters, but >>> what >>> >> >> would you do for List<String>? You would split a String using "\s" >>> as a >>> >> >> separator? >>> >> >> >>> >> >> Can you please mention a use case where a list oriented API is better >>> >> than >>> >> >> a string based one? >>> >> >> >>> >> >> Thanks, >>> >> >> Eduard >>> >> >> >>> >> >>> >>> >> >>> I'm ready to merge it into master so I'm waiting your vote. >>> >> >>> >>> >> >>> Caleb is it OK to put new stuff in master already or should I wait >>> the >>> >> >>> complete release to be done ? >>> >> >>> >>> >> >>> Here is my +1. >>> >> >>> >>> >> >>> Thanks, >>> >> >>> -- >>> >> >>> Thomas Mortagne >>> >> >>> >> _______________________________________________ >>> >> devs mailing list >>> >> [email protected] >>> >> http://lists.xwiki.org/mailman/listinfo/devs >>> >> >>> > _______________________________________________ >>> > devs mailing list >>> > [email protected] >>> > http://lists.xwiki.org/mailman/listinfo/devs >>> >>> >>> >>> -- >>> Thomas Mortagne >>> _______________________________________________ >>> devs mailing list >>> [email protected] >>> http://lists.xwiki.org/mailman/listinfo/devs >>> >> _______________________________________________ >> devs mailing list >> [email protected] >> http://lists.xwiki.org/mailman/listinfo/devs > > > > -- > Thomas Mortagne > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
