This was a massive effort - well done, Gil. My initial thought regarding the discussion about whether to commit all changes at once was initially - 'no way! we should break the changes up into multiple PRs'.
However after reviewing the first few files I see the intention behind the changes do not have any effect, and that the effort to now break the changes into multiple PRs feels like it would outweigh the benefit and we would risk demotivating anyone working on this task. I am happy to work through some of the files for review. I would suggest we have a process similar to: - Changes which do not affect behaviour are accepted. - Any changes which appear to be typos, might change the system's behaviour, or the reviewer is unsure about can have a review/comment raised against them. This will boost their visibility among other reviewers so they can be examined in a bit more depth. Does anyone know of a way in a GitHub PR that a reviewer can mark an individual file as reviewed-and-passed so that other reviewers can skip that file? On Sun, 22 Jan 2023 at 08:49, Jacques Le Roux <jacques.le.r...@les7arts.com> wrote: > Hi, > > Before I answered you I had an idea: we could split the effort. If we are > 10 to review it'd be reasonable. > > Jacques > > Le 21/01/2023 à 11:51, Gil Portenseigne a écrit : > > Haha, i understand, I will continue reviewing and testing while others > can review also, > > > > Thanks Jacques > > > > 21 janv. 2023 10:43:08 Jacques Le Roux <jacques.le.r...@les7arts.com>: > > > >> Thanks Gil, > >> > >> OK, seems good to me to avoid gstring indeed. > >> > >> I had a glance, I was too optimistic. I'll not review the 455(!) files > and will rather call our CTR mode as I'm much confident in your (big) work > :) > >> > >> +1 from my side > >> > >> Jacques > >> > >> > >> Le 21/01/2023 à 09:57, Gil Portenseigne a écrit : > >>> Yes, it is considered best practice to avoid gstring usage when not > needed. > >>> > >>> Like for others, we can decide to not apply this rule. > >>> > >>> The detailed rule from codenarc documentation : > >>> > >>> > >>> *UnnecessaryGString** Rule* > >>> > >>> /Since //CodeNarc// 0.13/ > >>> > >>> String objects should be created with single quotes, and GString > objects created with double quotes. Creating normal String objects with > double quotes is confusing to readers. > >>> > >>> Gil > >>> > >>> 21 janv. 2023 09:41:39 Jacques Le Roux <jacques.le.r...@les7arts.com>: > >>> > >>>> Hi Gil, > >>>> > >>>> So we need to use single quotes instead of double quotes now in > Groovy? > >>>> > >>>> Thanks > >>>> > >>>> Jacques > >>>> > >>>> Le 20/01/2023 à 17:01, Jacques Le Roux a écrit : > >>>>> Thank you very much Gil, > >>>>> > >>>>> +1 for a big squash... after some reviews... > >>>>> > >>>>> Jacques > >>>>> > >>>>> Le 20/01/2023 à 15:53, gil.portenseigne a écrit : > >>>>>> Hello Devs, > >>>>>> > >>>>>> That is with pleasure, that I managed to integrate into OFBiz > framework > >>>>>> (no plugins yet) Codenarc, and that the build is successful under > java > >>>>>> 17. > >>>>>> > >>>>>> > https://github.com/apache/ofbiz-framework/pull/517#issuecomment-1398487745 > >>>>>> > >>>>>> I tried to isolate rule fixes in separated commits, there are a lot > (133 > >>>>>> commits), with some redundancy. But rebasing is not easy since > files are > >>>>>> modified by several rule fixing. > >>>>>> > >>>>>> Integration and unit test are ok. I did some manual testing when I > got > >>>>>> some doubt, but it could be nice to have some more eyes on the > subject. > >>>>>> > >>>>>> After reviewing process, and if everything is fine, should we commit > >>>>>> that as a big squash ? > >>>>>> > >>>>>> WDYT > >>>>>> > >>>>>> Regards, > >>>>>> > >>>>>> Gil > -- Daniel Watford
