This is so useful, indeed! I'll start posting the links to my commits on github from now on when requesting dev reviews. Thanks, Om!
On 8 August 2014 18:47, OmPrakash Muppirala <bigosma...@gmail.com> wrote: > Our code is mirrored almost in real-time on to github. Github has > excellent code review features. We can use that to do our post-commit code > reviews. > > Here are the steps: > 1. Go to https://github.com/apache/flex-sdk > 2. In the 'branch' dropdown, select the branch: FLEX-34119 > 3. Click on the button labeled "xxx commits". It should show you all the > recent commits > 4. You can click on each commit to see its comments and code changes as a > diff. For example: > > https://github.com/apache/flex-sdk/commit/2f1b76827763b32988b1a1ccc73e603d9d7d788a > 5. When you mouse over each line, you will see a 'comment bubble' button > show up on the left. Click on it and enter your comments for that line. > > P.S. I am currently working with INFRA to make these comments to show up > on the dev@f.a.o list automatically. Until then, commenters/reviewers can > post on the dev list manually indicating that they have reviewed a commit. > > Hope this helps. > > Thanks, > Om > > > On Fri, Aug 8, 2014 at 9:32 AM, Frédéric THOMAS <webdoubl...@hotmail.com> > wrote: > > > For Crucible (code review), nothing we can do on our side, only INFRA > > could request it and I didn't ask them to do so (if you want to ask, it's > > up to you, it would be a very nice feature but you don't be in hurry), > only > > FishEye has been setup while ago > > https://fisheye6.atlassian.com/browse/flex-sdk > > Frédéric THOMAS > > > > > From: mihai.ch...@gmail.com > > > Date: Fri, 8 Aug 2014 17:21:04 +0100 > > > Subject: Re: [JIRA](ISSUE #FLEX-34119) Dev review request > > > CC: dev@flex.apache.org > > > > > > Frédéric, have you found out anything else regarding FishEye? I took a > > > look, but the Flex project isn't there (in fact, there seems to be > > > only one project, called "Default Project". It would be great to > > > review each other's code using this. > > > > > > Piotr, have you committed the TLF automatic test running code yet? It > > > would be great to run the test automatically in flex too, not least > > > because while trying to solve FLEX-34119 I had to create five separate > > > unit tests. > > > > > > Alex, I think I've finally nailed FLEX-34119! Since I first wrote > > > about it, it morphed into a total of five bugs: FLEX-34440, > > > FLEX-34424, FLEX-34456, FLEX-34458, and the original FLEX-34119. All > > > of them have unit tests, and now they all pass, which took a while. > > > (Wow, I have to say, HierarchicalCollectionViewCursor was pretty > > > broken.) > > > > > > There is a serious amount of code now on the FLEX-34119 branch[1]. It > > > would really help if you (and potentially others) could take another > > > look when you have time. Each commit has extensive information for > > > what was wrong, and what the solution is. Let me know if you think any > > > of those bugs could be solved in other ways. (Note that there are 7 or > > > 8 commits which are not mine; they were introduced when I merged with > > > develop.) > > > > > > > > > [1] > > > https://fisheye6.atlassian.com/graph/flex-sdk#branch=FLEX-34119&hl=Lineage&scrollToCsid=82d6b51694e846f0fb4e5505bf5a1b0601c21e7b > > > > > > On 22 July 2014 21:34, Alex Harui <aha...@adobe.com> wrote: > > > > Regarding code review, we do have tools but for most changes, if the > > tests pass you check it in and folks review the commit email. > > > > Sent via the PANTECH Discover, an AT&T 4G LTE smartphone. > > > > > > > > Mihai Chira <mihai.ch...@gmail.com> wrote: > > > > > > > > > > > > Hi Alex, > > > > > > > > > > > > thanks for taking a look. I was also surprised to discover not only > > > > that the parentTable variable wasn't necessary, but also that two > > > > entire if branches (for add and remove) *always* ended up throwing > the > > > > 'Bookmark no longer valid' error (see the commit message here[1] for > > > > details). In other words, they had never been tested. > > > > > > > > If there is some edge case, when we discover it we'll add a unit test > > > > to make sure it doesn't return. > > > > Speaking of which, I've just added the setItemAt operation to the > unit > > > > test, and I got some more 'Bookmark no longer valid' errors. I'll fix > > > > these new occurrences hopefully tomorrow. Once I do and once I run > the > > > > mustella tests, I'll ask you to take another look before I merge with > > > > develop. > > > > > > > > Two questions: > > > > > > > > 1. Would it be very complicated to run unit tests automatically in > the > > > > ant build script, or to the server CI process? > > > > 2. Also, how do we do code reviews? Do we use some tool, like fisheye > > > > or github, or just the IDE? I didn't find a wiki page about that. > > > > > > > > > > > > Thanks, > > > > Mihai > > > > > > > > > > > > [1] > > > https://fisheye6.atlassian.com/changelog/flex-sdk?cs=4cb80bfbb37225b86b9e69cc4ec2fc21efe0a668 > > > > > > > > On 22 July 2014 17:49, Alex Harui <aha...@adobe.com> wrote: > > > >> Hi Mihai, > > > >> > > > >> Thanks for taking on such a challenging goal. > > > >> > > > >> I don't see anything obviously wrong. IMO, the best thing to do > > would be > > > >> to set up mustella and run the Advanced DataGrid tests. That's what > > our > > > >> CI system will do when you finally check this into the develop > branch. > > > >> > > > >> It is a bit puzzling that the old code messed around with the parent > > stack > > > >> and it looks like you no longer need to. I wonder if there is some > > edge > > > >> case associated with that. > > > >> > > > >> -Alex > > > >> > > > >> On 7/22/14 4:00 AM, "Mihai Chira" <mihai.ch...@gmail.com> wrote: > > > >> > > > >>>On the branch "FLEX-34119" I committed two unit tests and a fix for > > > >>>this bug. If anyone would like to take a look, and run the tests, it > > > >>>would be really helpful, because it's a trickier area of the SDK. > > > >>>Could you let me know if: > > > >>>* you think the unit tests are in the right folder > > > >>>* the unit tests run as expected (see the README in > > > >>>HierarchicalCollectionViewCursor_FLEX_34119_Test for some > explanations > > > >>>about it) > > > >>>* it looks like the commits might break anything else > > > >>>* you think the variable renames in HierarchicalCollectionViewCursor > > are > > > >>>helpful > > > >>>* we could run the unit tests automatically on each ant build. Is > that > > > >>>possible now? > > > >>> > > > >>>I'm still adding an operation to the unit test (setItemAt), and > > > >>>looking to remove some duplicated code from > > > >>>HierarchicalCollectionViewCursor.collectionChangeHandler. > > > >>>I also plan to run the mustella tests soon, to make sure I haven't > > > >>>broken anything. > > > >>> > > > >>> > > > >>>Thank you! > > > >>>Mihai > > > >> > > > > >
