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
signature.asc
Description: PGP signature
