I think a PR with a single commit and a large number of files will be ok, if we make sure each usage of CS StringUtils functions is replaced by the Apache ones and their code removed from CS StringUtils. I also think that another PR could be created to move those functions mentioned by Rafael which should not be on StringUtils to new util classes and finally remove CS StringUtils from the codebase.
________________________________ From: Rafael Weingärtner <rafaelweingart...@gmail.com> Sent: Friday, January 12, 2018 6:59:01 AM To: dev Subject: Re: Which StringUtils to use? Well, there is always other approaches...If we did not use those static loggers, this number could be greatly reduced. Most of those objects are singletons and we could use a protected attribute in the first element of the hierarchy. I do not mind a PR with this number of files changes as long as you stick to a single change, what I mind is the combination of high number of files and commits.Then, at least for me, it becomes pretty hard to track down things. On Fri, Jan 12, 2018 at 6:19 AM, Daan Hoogland <daan.hoogl...@gmail.com> wrote: > if we don't use a wrapper we get PRs like > https://github.com/apache/cloudstack/pull/2276 in the future, trying to > update logging touches 1710 files. I think we should go for the wrapper > model on these kind of utilities. > > On Thu, Jan 11, 2018 at 9:59 PM, Rafael Weingärtner < > rafaelweingart...@gmail.com> wrote: > > > Wrapping would still hold code on our side. We have to get rid of code… > > > > If we want to start removing CloudStack’s StringUtils in favor of > > StringUtils from Apache, we could start creating PRs by components (java > > project in Eclipse). That is manageable to do and to review. There are > > about 119 classes that use CloudStack’s StringUtils. > > > > > > We will not be able to remove CloudStack's StringUtils though. There are > > very specific things there such as "applyPagination" that should not even > > be there... I guess the programmer was running out of places to write > code > > > > On Thu, Jan 11, 2018 at 6:25 PM, Daan Hoogland <daan.hoogl...@gmail.com> > > wrote: > > > > > All, I am having second thoughts. I think we should maintain a wrapper > > for > > > string utils and pass through as much as possible to commons string > > utils. > > > A similar thing is applicable to logging. It was started at one time > and > > a > > > second attempt was started to use slf4j. > > > I think we should encapsulate these kind of utilities to facilitate > > > migration. > > > There is also json and xml formatting and maybe handling sockets and > (big > > > one) data access objects :/ > > > > > > @Ron, all string utils are static methods. > > > > > > On Thu, Jan 11, 2018 at 12:11 AM, Ron Wheeler > > <rwheeler@artifact-software. > > > com> wrote: > > > > > > > Certainly better to find the references and remove them if you can > get > > > > that done in a single effort. > > > > > > > > Just a technical question: Could one not just add the Warning to the > > > > constructor? > > > > Might have to create a null (log warning only) constructor. > > > > > > > > Ron > > > > > > > > > > > > On 10/01/2018 3:58 PM, Daan Hoogland wrote: > > > > > > > >> We can add log messages to each of the methods in StringUtils but I > do > > > not > > > >> think that is a good way to go. Any method you touch you can reform > or > > > >> remove anyhow. > > > >> > > > >> On Wed, Jan 10, 2018 at 9:51 PM, Ron Wheeler < > > > >> rwhee...@artifact-software.com > > > >> > > > >>> wrote: > > > >>> Agreed about deprecation. > > > >>> A logged WARNing would be detected during testing as well as at > > > run-time. > > > >>> > > > >>> Ron > > > >>> > > > >>> On 10/01/2018 3:34 PM, Daan Hoogland wrote: > > > >>> > > > >>> Ron, we could but that would only log during compile-time, not on > > > >>> runtime. > > > >>> I am doing some analysis and commenting in Wido's ticket. > > > >>> > > > >>> On Wed, Jan 10, 2018 at 9:23 PM, Ron Wheeler > > > <rwheeler@artifact-software. > > > >>> com> wrote: > > > >>> > > > >>> Is it possible to mark it as deprecated and have it log a warning > > when > > > >>>> used? > > > >>>> > > > >>>> Ron > > > >>>> > > > >>>> > > > >>>> On 10/01/2018 2:26 PM, Daan Hoogland wrote: > > > >>>> > > > >>>> I think we could start with giving it an explicit non standard > name > > > like > > > >>>>> CloudStackLocalStringUtils or something a little shorter. Making > > sure > > > >>>>> that > > > >>>>> we prefer for these types of utils to be imported from other > > > projects. > > > >>>>> > > > >>>>> On Wed, Jan 10, 2018 at 4:26 PM, Wido den Hollander < > > w...@widodh.nl> > > > >>>>> wrote: > > > >>>>> > > > >>>>> > > > >>>>> On 01/10/2018 01:09 PM, Rafael Weingärtner wrote: > > > >>>>>> > > > >>>>>> Instead of creating a PR for that, we could do the bit by bit > job > > > >>>>>> > > > >>>>>>> (hopefully one day we finish the job). > > > >>>>>>> Every time we see a code using ACS's StringUtils, we check if > it > > > can > > > >>>>>>> be > > > >>>>>>> replaced by Apache's one. > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> Yes, but that will slip from peoples attention and we will > > probably > > > >>>>>>> see > > > >>>>>>> > > > >>>>>> cases where people still use the old one by accident. > > > >>>>>> > > > >>>>>> I've created a issue: https://issues.apache.org/jira > > > >>>>>> /browse/CLOUDSTACK-10225 > > > >>>>>> > > > >>>>>> I also started on some low hanging fruit as some methods in > > > >>>>>> StringUtils > > > >>>>>> are not used or are very easy to replace. > > > >>>>>> > > > >>>>>> > > > >>>>>> Wido > > > >>>>>> > > > >>>>>> On Wed, Jan 10, 2018 at 10:01 AM, Wido den Hollander < > > > w...@widodh.nl> > > > >>>>>> > > > >>>>>> wrote: > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> On 01/10/2018 12:01 PM, Daan Hoogland wrote: > > > >>>>>>> > > > >>>>>>>> I'd say remove as much functionality as we can from 'our' > > > >>>>>>>> StringUtils > > > >>>>>>>> and > > > >>>>>>>> > > > >>>>>>>> phase them out asap. > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> Yes, but such a PR would be invasive and would be difficult > to > > > >>>>>>>>> merge > > > >>>>>>>>> and > > > >>>>>>>>> > > > >>>>>>>>> also break a lot of other code. > > > >>>>>>>> > > > >>>>>>>> It's not easy since it will touch a lot, but I mean, a lot of > > > files. > > > >>>>>>>> > > > >>>>>>>> Our StringUtils was a very good solution, but the Apache one > is > > > >>>>>>>> better I > > > >>>>>>>> think. > > > >>>>>>>> > > > >>>>>>>> Wido > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>> On Wed, Jan 10, 2018 at 11:59 AM, Wido den Hollander < > > > >>>>>>>> w...@widodh.nl> > > > >>>>>>>> > > > >>>>>>>> wrote: > > > >>>>>>>> > > > >>>>>>>>> Hi, > > > >>>>>>>>> > > > >>>>>>>>> We have com.cloud.utils.StringUtils which has a few nice > > > functions, > > > >>>>>>>>> > > > >>>>>>>>>> but > > > >>>>>>>>>> throughout the code I also see > org.apache.commons.lang.String > > > >>>>>>>>>> Utils > > > >>>>>>>>>> > > > >>>>>>>>>> They both provide about the same functionality, but which > one > > do > > > >>>>>>>>>> we > > > >>>>>>>>>> prefer? > > > >>>>>>>>>> > > > >>>>>>>>>> I'd say org.apache.commons.lang.StringUtils as that allows > us > > > to > > > >>>>>>>>>> remove > > > >>>>>>>>>> our own StringUtils, but we could also have 'our' > StringUtils > > > >>>>>>>>>> simply > > > >>>>>>>>>> be a > > > >>>>>>>>>> wrapper around org.apache.commons.lang.StringUtils > > > >>>>>>>>>> > > > >>>>>>>>>> Opinions? > > > >>>>>>>>>> > > > >>>>>>>>>> Wido > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>> -- > > > >>>> Ron Wheeler > > > >>>> President > > > >>>> Artifact Software Inc > > > >>>> email: rwhee...@artifact-software.com > > > >>>> skype: ronaldmwheeler > > > >>>> phone: 866-970-2435, ext 102 > > > >>>> > > > >>>> > > > >>>> > > > >>> -- > > > >>> Daan > > > >>> > > > >>> > > > >>> -- > > > >>> Ron Wheeler > > > >>> President > > > >>> Artifact Software Inc > > > >>> email: rwhee...@artifact-software.com > > > >>> skype: ronaldmwheeler > > > >>> phone: 866-970-2435, ext 102 > > > >>> > > > >>> > > > >>> > > > >> > > > > > > > > -- > > > > Ron Wheeler > > > > President > > > > Artifact Software Inc > > > > email: rwhee...@artifact-software.com > > > > skype: ronaldmwheeler > > > > phone: 866-970-2435, ext 102 > > > > > > > > > > > > > > > > > -- > > > Daan > > > > > > > > > > > -- > > Rafael Weingärtner > > > > > > -- > Daan > -- Rafael Weingärtner nicolas.vazq...@shapeblue.com www.shapeblue.com , @shapeblue
