On Wed, May 30, 2012 at 4:50 PM, Marius Dumitru Florea <[email protected]> wrote: > 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.
The DiffConfiguration and MergeConfiguration are more future stuff in which I forgot to put generic map for at least custom setup even if the current implementation does not really have much setup possibilities. I have some ideas of things to put in MergeConfiguration already (priority to current or next version in case of conflict, etc.) but did not had time to implement them already and DiffConfiguration is mostly here for consistency but I can remove it. null is simply supported as "I don't have any 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. This was actually mostly to indicate to the implementation a kind of top logqueue to update along with the current mergeresult. I forgot to removed it since I planned to let the API client copy the log from the returned merge result if needed intead. > > (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. It's not really specific IMO, it's just one way to represent a diff between two versions. The idea is to provide a bunch of diff representations in the DiffResult that the implementation is creating the way it fit the best for it. We can imagine some library which actually create a unified diff (JDiff do it that way for example) from which you would create a lighter patch oriented diff. > > 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 -- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
