You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Laszlo Hornyak <la...@gmail.com> on 2013/06/09 21:47:33 UTC
Review Request: use commons-lang StringUtils
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11767/
-----------------------------------------------------------
Review request for cloudstack.
Description
-------
commons-lang is already a transitive dependency of the utils project, which allows removing some duplicated functionality.
This patch replaces StringUtils.join(String, Object...) with it's commons-lang counterpart.
It also replaces calls to String join(Iterable<? extends Object>, String) in cases where an array is already exist and it is only wrapped into a List.
Diffs
-----
server/src/com/cloud/storage/s3/S3ManagerImpl.java 61e5573
services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java e7fa5b2
utils/src/com/cloud/utils/S3Utils.java b7273a1
utils/src/com/cloud/utils/StringUtils.java 14ff4b1
utils/test/com/cloud/utils/StringUtilsTest.java 3c162c7
Diff: https://reviews.apache.org/r/11767/diff/
Testing
-------
- Unit test added
Thanks,
Laszlo Hornyak
RE: Review Request: use commons-lang StringUtils
Posted by Alex Huang <Al...@citrix.com>.
Ok...please resubmit.
--Alex
> -----Original Message-----
> From: Laszlo Hornyak [mailto:noreply@reviews.apache.org] On Behalf Of
> Laszlo Hornyak
> Sent: Tuesday, June 11, 2013 11:48 AM
> To: cloudstack; Laszlo Hornyak; Alex Huang
> Subject: Re: Review Request: use commons-lang StringUtils
>
>
>
> > On June 10, 2013, 11:56 p.m., Alex Huang wrote:
> > > The patch did not apply cleanly. Please resubmit. Since you have to
> resubmit anyways, I think why not just change all of the code to use the
> commons.lang version. I see no point in keeping the method in StringUtils if
> it's replaceable by one in a standard library.
> > >
>
> Hi Alex,
>
> I think it makes sense to keep the StringUtils.join(String, Object...) method
> because it allows to use varargs, unlike the one in commons-lang, and the
> code somewhat builds on it.
>
>
> - Laszlo
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11767/#review21678
> -----------------------------------------------------------
>
>
> On June 9, 2013, 7:47 p.m., Laszlo Hornyak wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/11767/
> > -----------------------------------------------------------
> >
> > (Updated June 9, 2013, 7:47 p.m.)
> >
> >
> > Review request for cloudstack.
> >
> >
> > Description
> > -------
> >
> > commons-lang is already a transitive dependency of the utils project, which
> allows removing some duplicated functionality.
> > This patch replaces StringUtils.join(String, Object...) with it's commons-lang
> counterpart.
> > It also replaces calls to String join(Iterable<? extends Object>, String) in
> cases where an array is already exist and it is only wrapped into a List.
> >
> >
> > Diffs
> > -----
> >
> > server/src/com/cloud/storage/s3/S3ManagerImpl.java 61e5573
> > services/secondary-
> storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorage
> Resource.java e7fa5b2
> > utils/src/com/cloud/utils/S3Utils.java b7273a1
> > utils/src/com/cloud/utils/StringUtils.java 14ff4b1
> > utils/test/com/cloud/utils/StringUtilsTest.java 3c162c7
> >
> > Diff: https://reviews.apache.org/r/11767/diff/
> >
> >
> > Testing
> > -------
> >
> > - Unit test added
> >
> >
> > Thanks,
> >
> > Laszlo Hornyak
> >
> >
Re: Review Request: use commons-lang StringUtils
Posted by Laszlo Hornyak <la...@gmail.com>.
> On June 10, 2013, 11:56 p.m., Alex Huang wrote:
> > The patch did not apply cleanly. Please resubmit. Since you have to resubmit anyways, I think why not just change all of the code to use the commons.lang version. I see no point in keeping the method in StringUtils if it's replaceable by one in a standard library.
> >
Hi Alex,
I think it makes sense to keep the StringUtils.join(String, Object...) method because it allows to use varargs, unlike the one in commons-lang, and the code somewhat builds on it.
- Laszlo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11767/#review21678
-----------------------------------------------------------
On June 9, 2013, 7:47 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11767/
> -----------------------------------------------------------
>
> (Updated June 9, 2013, 7:47 p.m.)
>
>
> Review request for cloudstack.
>
>
> Description
> -------
>
> commons-lang is already a transitive dependency of the utils project, which allows removing some duplicated functionality.
> This patch replaces StringUtils.join(String, Object...) with it's commons-lang counterpart.
> It also replaces calls to String join(Iterable<? extends Object>, String) in cases where an array is already exist and it is only wrapped into a List.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/s3/S3ManagerImpl.java 61e5573
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java e7fa5b2
> utils/src/com/cloud/utils/S3Utils.java b7273a1
> utils/src/com/cloud/utils/StringUtils.java 14ff4b1
> utils/test/com/cloud/utils/StringUtilsTest.java 3c162c7
>
> Diff: https://reviews.apache.org/r/11767/diff/
>
>
> Testing
> -------
>
> - Unit test added
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request: use commons-lang StringUtils
Posted by Alex Huang <al...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11767/#review21678
-----------------------------------------------------------
The patch did not apply cleanly. Please resubmit. Since you have to resubmit anyways, I think why not just change all of the code to use the commons.lang version. I see no point in keeping the method in StringUtils if it's replaceable by one in a standard library.
- Alex Huang
On June 9, 2013, 7:47 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11767/
> -----------------------------------------------------------
>
> (Updated June 9, 2013, 7:47 p.m.)
>
>
> Review request for cloudstack.
>
>
> Description
> -------
>
> commons-lang is already a transitive dependency of the utils project, which allows removing some duplicated functionality.
> This patch replaces StringUtils.join(String, Object...) with it's commons-lang counterpart.
> It also replaces calls to String join(Iterable<? extends Object>, String) in cases where an array is already exist and it is only wrapped into a List.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/s3/S3ManagerImpl.java 61e5573
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java e7fa5b2
> utils/src/com/cloud/utils/S3Utils.java b7273a1
> utils/src/com/cloud/utils/StringUtils.java 14ff4b1
> utils/test/com/cloud/utils/StringUtilsTest.java 3c162c7
>
> Diff: https://reviews.apache.org/r/11767/diff/
>
>
> Testing
> -------
>
> - Unit test added
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request: use commons-lang StringUtils
Posted by Chip Childers <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11767/#review21998
-----------------------------------------------------------
Ship it!
Applied to master: c88d8fb3a2f6c418c6c7af8ff702a93bcdb2d752
Thanks for the patch!
- Chip Childers
On June 15, 2013, 4:32 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11767/
> -----------------------------------------------------------
>
> (Updated June 15, 2013, 4:32 p.m.)
>
>
> Review request for cloudstack.
>
>
> Description
> -------
>
> commons-lang is already a transitive dependency of the utils project, which allows removing some duplicated functionality.
> This patch replaces StringUtils.join(String, Object...) with it's commons-lang counterpart.
> It also replaces calls to String join(Iterable<? extends Object>, String) in cases where an array is already exist and it is only wrapped into a List.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/s3/S3ManagerImpl.java 61e5573
> services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java e7fa5b2
> utils/src/com/cloud/utils/S3Utils.java b7273a1
> utils/src/com/cloud/utils/StringUtils.java 14ff4b1
> utils/test/com/cloud/utils/StringUtilsTest.java 3c162c7
>
> Diff: https://reviews.apache.org/r/11767/diff/
>
>
> Testing
> -------
>
> - Unit test added
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request: use commons-lang StringUtils
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11767/
-----------------------------------------------------------
(Updated June 15, 2013, 4:32 p.m.)
Review request for cloudstack.
Changes
-------
rebased
Description
-------
commons-lang is already a transitive dependency of the utils project, which allows removing some duplicated functionality.
This patch replaces StringUtils.join(String, Object...) with it's commons-lang counterpart.
It also replaces calls to String join(Iterable<? extends Object>, String) in cases where an array is already exist and it is only wrapped into a List.
Diffs (updated)
-----
server/src/com/cloud/storage/s3/S3ManagerImpl.java 61e5573
services/secondary-storage/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java e7fa5b2
utils/src/com/cloud/utils/S3Utils.java b7273a1
utils/src/com/cloud/utils/StringUtils.java 14ff4b1
utils/test/com/cloud/utils/StringUtilsTest.java 3c162c7
Diff: https://reviews.apache.org/r/11767/diff/
Testing
-------
- Unit test added
Thanks,
Laszlo Hornyak