Hi Daan,
I didn't update the patch after the couple of works because I wanted to do
it with others Chiradeep asked if any.
Let me know when you want me to.
Thanks
Alex Ough
On Tue, Mar 11, 2014 at 4:37 PM, Daan Hoogland <daan.hoogl...@gmail.com>wrote:
> I will call the merge in a moment, but won't do it immediately.
>
> Be there for me when any issues come up. If not I will revert it.
>
> @Chiradeep: did Alex answer your concerns adequately?
>
>
> kind regards,
>
> On Tue, Mar 11, 2014 at 6:43 PM, Alex Ough <alex.o...@sungard.com> wrote:
> > 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
> >>>
> >>>
> >>>
> >>
>
>
>
> --
> Daan
>
>
