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 for. The comment against JobDetails.groovy would hopefully get another reviewer to take a look and provide a bit more info. This gives me a chance to learn something! :) The comments in the PR are part of my ongoing review. Since the review has not been submitted yet, perhaps it's not visible to you? If that's the case then we'll need to submit multiple reviews as we go. Please could you let me know if my review within PR 517 is visible to you. Thanks, Dan. On Mon, 6 Feb 2023 at 12:35, gil.portenseigne <gil.portensei...@nereide.fr> wrote: > 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 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 had added a comment to the PR, but > > depending on what the issue is, adding a note to the table might be > > appropriate. > > > > I think comments in PRs are a reasonable approach since they will keep > the > > comment alongside the code that needs to be worked on or discussed. I > would > > rather not duplicate comments from the PR in the tracking document. > > > > Thanks, > > > > Dan. > > > > On Mon, 6 Feb 2023 at 08:18, gil.portenseigne < > gil.portensei...@nereide.fr> > > wrote: > > > > > 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 week in the review process. > > > > > > Regards, > > > > > > Gil > > > > > > On 28/01/23 08:46, Daniel Watford wrote: > > > > 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 <d...@foomoo.co.uk> > wrote: > > > > > > > > > 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 > > > > > below as PASSED. > > > > > - If the reviewer identifies an issue with a changed file, then > they > > > > > should add a comment in the PR on GitHub AND mark the > corresponding > > > entry > > > > > in the table below as WORK NEEDED. > > > > > - If the reviewer is unsure how to classify a changed file they > > > should > > > > > mark the corresponding entry in the table below as UNSURE. > > > > > - In each of the above cases, the reviewer should add their name > > > > > against the entry in the table below. > > > > > > > > > > The checklist doesn't give us the opportunity to see what files > need > > > some > > > > > additional help. > > > > > > > > > > I'm sure there must be some way of getting Confluence to produce a > > > table > > > > > from a list - I just don't seem to have found it yet! I'll play > around > > > with > > > > > Confluence a bit more. > > > > > > > > > > But as mentioned before, perhaps I am making too much out of > tracking > > > this > > > > > review. > > > > > > > > > > Thanks, > > > > > > > > > > Dan. > > > > > > > > > > > > > > > On Fri, 27 Jan 2023 at 17:05, gil.portenseigne < > > > > > gil.portensei...@nereide.fr> 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, 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 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 to track reviewed > files. > > > > >> > > > > > > >> > > I created a page here - Codenarc integration review tracker - > > > OFBiz > > > > >> Project > > > > >> > > Open Wiki - Apache Software Foundation > > > > >> > > < > > > > >> > > > > https://cwiki.apache.org/confluence/display/OFBIZ/Codenarc+integration+review+tracker > > > > >> > > > > > >> > > - > > > > >> > > suggesting an approach. > > > > >> > > > > > > >> > > If the approach is acceptable then all reviewers should be > able to > > > > >> update > > > > >> > > the page as we go. > > > > >> > > > > > > >> > > I'm stuck with finding a nice way to generate a table listing > all > > > the > > > > >> > > changed files and the review status of each file. I have > included > > > the > > > > >> > > commands to produce the list of files and shown some examples > of > > > how > > > > >> to add > > > > >> > > a header, but my attempts to turn that into something useful > on a > > > > >> > > confluence page have not been fruitful. > > > > >> > > > > > > >> > > So two questions. > > > > >> > > - Is it worth coming up with a page/table to track this PR or > am I > > > > >> just > > > > >> > > creating unnecessary admin work when we could use comments in > the > > > PR? > > > > >> > > - Can anyone create a table in Confluence that we could use to > > > track > > > > >> the > > > > >> > > review effort? > > > > >> > > > > > > >> > > Thanks, > > > > >> > > > > > > >> > > Dan. > > > > >> > > > > > > >> > > > > > > >> > > On Fri, 27 Jan 2023 at 15:27, gil.portenseigne < > > > > >> gil.portensei...@nereide.fr> > > > > >> > > wrote: > > > > >> > > > > > > >> > > > 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 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 27/01/23 12:00, Jacques Le Roux wrote: > > > > >> > > > > > > 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 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 start of > > > each > > > > >> file, to > > > > >> > > > let > > > > >> > > > > > > > others know that review job has been done. > > > > >> > > > > > > > > > > > >> > > > > > > > WDYT ? > > > > >> > > > > > > > > > > > >> > > > > > > > Gil > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > On 26/01/23 07:48, Jacques Le Roux wrote: > > > > >> > > > > > > > > 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 Watford a écrit : > > > > >> > > > > > > > > > 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? > > > > >> > > > > > > > >> > > > > > > >> > > > > > > >> > > -- > > > > >> > > Daniel Watford > > > > >> > > > > >> > > > > >> > > > > > > > > > > -- > > > > > Daniel Watford > > > > > > > > > > > > > > > > > -- > > > > Daniel Watford > > > > > > > > > -- > > Daniel Watford > -- Daniel Watford
