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