I worked on a couple of items, so can you give me the confirmation for the
rest items so that I can wrap this up?
I really want to include this into 4.4.
Thanks
Alex Ough
On Mon, Mar 10, 2014 at 3:41 PM, Alex Ough <alex.o...@sungard.com> wrote:
> Please see my reply/question in blue.
>
> Thanks
> Alex Ough
>
>
> On Mon, Mar 10, 2014 at 1:25 PM, Chiradeep Vittal <
> chiradeep.vit...@citrix.com> wrote:
>
>> I haven¹t looked at it because it is huge. But preliminary scan:
>>
>> - there are unit tests missing for changes to core CS code
>>
> Unit tests for only new classes were added because I couldn't
> find unit tests of the ones I modified
>
>
>> - uses com.amazonaws.util.json (why?)
>>
> They are used to handle the json objects. Let me know if you
> want me to replace it with another.
>
> - code format does not follow coding convention ( placement of {}, camel
>> case api_interface )
>>
> Done
>
>
>> - package namespace is com.cloud instead of org.apache for new files
>>
> I didn't know that. So do I need to use 'org.apache' package for
> all new classes?
>
> - no file-level comments. What does LocalAccountManager do? Why does it
>> exist? Etc.
>>
> Done.
>
>
>> - No interfaces, only abstract classes. No use of @Inject. While this
>> might be OK, it does make it harder to test and does not follow the rest
>> of ACS conventions.
>>
> I don't think there is any interface, but let me know if you find
> any.
> Any recommendation to replace @inject?
>
>>
>> I would urge the submitter to break up the submission.
>> A) the changes to CS core, with explanations / comments and tests
>> B) the sync service should be an interface with concrete implementations
>> in its own package
>> C) more tests
>>
> can you give me a little specific what kind of tests you need
> more?
>
>
>
>> On 3/10/14, 3:37 AM, "Daan Hoogland" <daan.hoogl...@gmail.com> wrote:
>>
>> >Hi everyody,
>> >
>> >The people at sungard have been making this change for 4.4 and I want
>> >to merge/apply it this week. It is more then a screenfull and might
>> >cause issue is a setup or two.
>> >
>> >have a look at https://reviews.apache.org/r/17790/
>> >
>> >a ref to ticket and fs page are in the review request.
>> >
>> >kind regards,
>> >--
>> >Daan
>>
>>
>>
>
