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
