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
