chemistry-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gavin Cornwell <gavin.cornw...@alfresco.com>
Subject Re: Browser binding re-factoring
Date Tue, 06 May 2014 09:13:00 GMT
Hi Peter,

Thanks for your feedback, I’ve just committed some of these changes. I’ve also renamed
the get**Url methods on CMISBrowserBaseService to follow Apple’s naming recommendations
[1], hope this is OK with you.

I will try and complete the HttpUploadRequest change today as well.

Regards,

Gavin

[1] https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingMethods.html


On 5 May 2014, at 07:55, Sutter, Peter <peter.sutter@sap.com> wrote:

> Hi Gavin,
> 
> yes agree with your second proposal (supply start/end data with flag for
> HttpUploadRequest).
> 
> I moved the media type constants there because I needed
> kCMISMediaTypeOctetStream also for browser binding. And OpenCMIS library
> has those constants also in the
> org.apache.chemistry.opencmis.commons.impl.Constants class. So at least
> the constant kCMISMediaTypeOctetStream should be there. Don’t mind if you
> want to move the others back.
> 
> We have a similar method in our coding which is on a NSString category but
> it’s also fine for me if we change it back to a private method on the
> CMISURLUtil class. 
> 
> 
> Best regards,
> Peter
> 
> On 5/2/14, 5:33 PM, "Gavin Cornwell" <gavin.cornwell@alfresco.com> wrote:
> 
>> Hi,
>> 
>> I’ve completed the agreed refactoring on the browser binding branch and
>> also fixed the underlying issues causing the last remaining tests to
>> fail. With the fixes in place all tests are now passing against both
>> bindings. I’ve therefore changed the test config file to run the tests
>> against both bindings by default.
>> 
>> To fix the JSON parsing issue I’ve added a hack that should be by-passed
>> if Apple ever fix the bug. We now remove the minValue and maxValue
>> properties from the JSON if a parsing error occurs and re-try and parse
>> operation.
>> 
>> I took a look at CMISHttpUploadRequest, I agree we need to move the
>> prepareXMLWithCMISProperties method out, we could introduce an elaborate
>> mechanism where custom HttpUpload/DownloadRequest objects could be
>> plugged into the network provider, but I think the easiest/most sensible
>> thing to do is to just expose the underlying base64Encoding flag as a
>> parameter and use the combinedStream stream feature you’ve added. Both
>> bindings can then supply the start/end data and set the flag
>> appropriately, what do you think?
>> 
>> A couple of other questions following the changes provided in CMIS-794...
>> 
>> Was there a particular reason why the atom pub specific media type
>> constants (kCMISMediaTypeService etc.) got moved into the common
>> CMISConstants file? I personally think these should be moved back to
>> CMISAtomPubConstants as they’re only used by that binding, do you agree?
>> 
>> Finally, was there a particular reason to make the replacePathWithPath
>> method in CMISURLUtil a category? It’s only used in this class so just
>> wondering why it’s not a simple private method?
>> 
>> Have a great weekend.
>> 
>> Regards,
>> 
>> Gavin
>> 
>> 
>> 
>> On 1 May 2014, at 16:35, Gavin Cornwell <gavin.cornwell@alfresco.com>
>> wrote:
>> 
>>> Hi Peter,
>>> 
>>> Excellent.
>>> 
>>> I have assigned CMIS-794 to myself, I’ll commit this tomorrow as well.
>>> 
>>> I will also take a look at the CMISHttpUploadRequest issue you mention.
>>> 
>>> Regards,
>>> 
>>> Gavin
>>> 
>>> 
>>> 
>>> On 1 May 2014, at 15:53, Sutter, Peter <peter.sutter@sap.com> wrote:
>>> 
>>>> Hi Gavin,
>>>> 
>>>> I just uploaded the current state of my implementation:
>>>> https://issues.apache.org/jira/browse/CMIS-794
>>>> 
>>>> Now all methods of the interfaces should be implemented. I have also
>>>> tested the browser binding implementation in our app and it works very
>>>> nicely as far as I can see.
>>>> 
>>>> There is maybe one area where I¹m not really sure because the
>>>> CMISHttpUploadRequest class contains atom pub specific coding
>>>> (prepareXMLWithCMISProperties) which should probably be moved down to
>>>> the
>>>> respective atom class.
>>>> And still there is the JSONSerializer bug with very large/small double
>>>> values where we need to decide how we deal with it.
>>>> 
>>>> You can start with your part 2 refactoring, no problem. Will only do
>>>> minor
>>>> changes in the next working days, if any.
>>>> 
>>>> Best regards,
>>>> Peter
>>>> 
>>>> 
>>>> On 5/1/14, 11:03 AM, "Gavin Cornwell" <gavin.cornwell@alfresco.com>
>>>> wrotsue:
>>>> 
>>>>> Hi Peter,
>>>>> 
>>>>> Great work on the browser binding so far, it¹s really coming together.
>>>>> 
>>>>> As you¹ve probably seen by now, I did part 1 of the re-factoring we
>>>>> agreed upon previously (updating property names).
>>>>> 
>>>>> Part 2 is going to be a bit more disruptive as it¹s going to involve
>>>>> renaming and moving quite a few classes (ensure all Atom classes are
>>>>> named as such). So I thought it would be best to co-ordinate this with
>>>>> you to avoid a merge headache for you and I.
>>>>> 
>>>>> I was planning on doing this tomorrow, do you think you¹ll have your
>>>>> latest set of changes committed by then?
>>>>> 
>>>>> As a side note, I looked into the UriBuilder classes last night and
>>>>> found, as you¹d hinted at, they are pretty specific to the AtomPub
>>>>> binding so I¹m going to rename them as such and move the constants
>>>>> from
>>>>> the generic CMISBindingSession class to the CMISAtomPubConstants
>>>>> class.
>>>>> 
>>>>> Following this, I¹ll be available to help out with the actual
>>>>> implementation, if there are any areas you¹d like me to look at (that
>>>>> are
>>>>> not on your critical path) please let me know.
>>>>> 
>>>>> Regards,
>>>>> 
>>>>> Gavin
>>>> 
>>> 
>> 
> 


Mime
View raw message