Everybody can now help by looking at
https://nightlies.apache.org/ofbiz/trunk/groovyScripts.html (only trunk for now)
Le 12/04/2023 à 17:24, Jacques Le Roux a écrit :
Hi Gil,
IMO better forget it, that's not a big deal and it looks good enough to me at
Hi Gil,
IMO better forget it, that's not a big deal and it looks good enough to me at
https://issues.apache.org/jira/browse/OFBIZ-11167
Jacques
Le 12/04/2023 à 16:58, Gil Portenseigne a écrit :
Hello !
I just squashed and committed the pull request, I would like to thank you again
for the
Hello !
I just squashed and committed the pull request, I would like to thank
you again for the review work and animation !
I failed the commit message due to the pull request feature i was not
familiar about...
I am not aware of "force push" policy in trunk that could allow me to
fix
Hi Guys,
For those who have used a non "PASSED" lozenge in wiki and resolved a related
conversation in GH please update the status in wiki
TIA
Jacques
Le 28/01/2023 à 11:51, Gil Portenseigne a écrit :
Oh sorry indeed i overview the review approach section.
The table is nice, thanks Dan !
No, we should be able to create multiple reviews within a single PR - or
perhaps add a comment to an existing review.
On Tue, 7 Feb 2023 at 17:44, Jacques Le Roux
wrote:
> Le 07/02/2023 à 16:53, gil.portenseigne a écrit :
> > yes ongoing review are private, you need to finish it to let other
Le 07/02/2023 à 16:53, gil.portenseigne a écrit :
yes ongoing review are private, you need to finish it to let other see
your comments and suggestions.
Does it mean that you need to review ALL before others see what you commented
?
Thanks
Jacques
Hello Dan,
Inline
On 07/02/23 03:21, Daniel Watford wrote:
> Hi Gil,
>
> There is a comment 'Can requestIdMap be removed?' against the
> StatsSinceStart.groovy file in the PR. This comment is part of a review.
>
> JobDetails.groovy was marked as UNSURE as it concerned a possible feature
> of
Hi Gil,
There is a comment 'Can requestIdMap be removed?' against the
StatsSinceStart.groovy file in the PR. This comment is part of a review.
JobDetails.groovy was marked as UNSURE as it concerned a possible feature
of Groovy that I wasn't aware of and couldn't find appropriate
documentation
Indeed, i agree it is better in PR (sorry again for missing it), but i
did not find comments about the StatsSinceStart.groovy and other files
you noted as unpassed.
Did I miss something ?
Gil
On 06/02/23 09:10, Daniel Watford wrote:
> Hi Gil,
>
> The Review Approach section of the tracking
Le 06/02/2023 à 09:18, gil.portenseigne a écrit :
I will try to advance this week in the review process.
Hi Gil, Daniel, All,
I'll try to the apply the 10 min principle to the review. Reviewing 5
documents/day should allow to do my part (150) in roughly a month :)
Jacques
Hi Gil,
The Review Approach section of the tracking document says that comments
should be added to the PR when marking a file as WORK_NEEDED. In my case I
added review comments to the PR.
However I didn't specify how a reviewer should communicate why they were
UNSURE about any particular file. I
Hello Daniel,
Thanks again for the review you did, could we add a small description
when UNSURE or WORK_NEEDED is set in the review table ?
Or will it be best to use github comments in pull request ?
I'm curious about the reason, and would like to help solve them.
I will try to advance this
Oh sorry indeed i overview the review approach section.
The table is nice, thanks Dan !
28 janv. 2023 09:37:50 Daniel Watford :
> Hi Gil,
>
> I don't think a checklist is quite enough, assuming we want to track the
> status of each file reviewed.
>
> From the review approach section:
>
>
>
Thanks Daniel,
Looks great, but indeed a bit more work, notably because you need to C/P,
modify and save.
Also looks like we are still only 3 to review, so about 150 files each. Some
help would be appreciated :)
Jacques
Le 28/01/2023 à 09:46, Daniel Watford a écrit :
Turns out I was able
Turns out I was able to import the list of files into Excel and copy and
paste the table from Excel to Confluence.
On Sat, 28 Jan 2023 at 08:37, Daniel Watford wrote:
> Hi Gil,
>
> I don't think a checklist is quite enough, assuming we want to track the
> status of each file reviewed.
>
> From
Hi Gil,
I don't think a checklist is quite enough, assuming we want to track the
status of each file reviewed.
>From the review approach section:
- If in the reviewers opinion a file change will not change OFBiz
behaviour in any way they should mark the corresponding entry in the table
Good enough for me, I checked the 1st in the list after reviewing (no need to
modify, cool)
Happy weekend
Le 27/01/2023 à 18:05, gil.portenseigne a écrit :
I got to leave, but i generated in confluence a list of check, is that
good enough ?
Gil
On 27/01/23 05:41, gil.portenseigne wrote:
I got to leave, but i generated in confluence a list of check, is that
good enough ?
Gil
On 27/01/23 05:41, gil.portenseigne wrote:
> Hello, indeed, that will generate much spam, i did some before reading
> your answer.
>
> I'll have a look for conluence.
>
> Gil
>
>
> On 27/01/23 04:14,
Hello, indeed, that will generate much spam, i did some before reading
your answer.
I'll have a look for conluence.
Gil
On 27/01/23 04:14, Daniel Watford wrote:
> Hi Gill and Jacques,
>
> I don't think we should add comments to the PR to track the files that we
> have reviewed as I think each
Hi Gill and Jacques,
I don't think we should add comments to the PR to track the files that we
have reviewed as I think each comment will appear separately in the PR's
conversation view.
However, with such a large PR where we hope to get several reviewers
involved I think we do need a mechanism
Oops, i did a fixup commit with push force that remove all comments in
the pull request... Will not do that again.
I fixed the detected typo.
gil
On 27/01/23 02:56, Jacques Le Roux wrote:
> Ah OK, sounds better indeed
>
> Le 27/01/2023 à 14:06, gil.portenseigne a écrit :
> > The idea is not to
Ah OK, sounds better indeed
Le 27/01/2023 à 14:06, gil.portenseigne a écrit :
The idea is not to modify the files, but to add a comment into the pull
request. Those allowing each reviewer to check the viewed checkbox if a
comment is present, to collapse already reviewed files.
So no need
The idea is not to modify the files, but to add a comment into the pull
request. Those allowing each reviewer to check the viewed checkbox if a
comment is present, to collapse already reviewed files.
So no need further action, apart the real code modification request,
when commiting the code.
On
Hi Gil, Daniel,
I agree Gil, I just tried before seeing your message and came to the same
conclusion.
With a comment at top we would need to remove it later, right? Could be easy if
it's the same unique words in every file.
Jacques
Le 27/01/2023 à 10:41, gil.portenseigne a écrit :
Hi
Hi Daniel, Jacques,
I wonders the same, the "Review changes" do not seems to concern one
file but the whole pull request, there is a review checkbox, but it
seems to be personal, i checked the first one
(AcctgAdminServices.groovy) for testing purpose.
What we could do is to add a comment at the
Hi Daniel,
In "Files changed" tab*, when you select a file, the "Review changes" button
allows you to comment, approve or request changes on this file.
I guess "approve" is what you are looking for?
* https://github.com/apache/ofbiz-framework/pull/517/files
Le 26/01/2023 à 17:26, Daniel
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
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
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 :
> 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
>
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
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
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,
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.
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
34 matches
Mail list logo
