On Wed, May 30, 2012 at 5:09 PM, Thomas Mortagne <[email protected]> wrote: > 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".
Actually thoses are not supposed to be interfaces. That's a mistake, I skipped this part of your question. > >> >> (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 -- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
