fu: Close-up of Fu, bringing a scoop of water to her mouth (Default)
[personal profile] fu

There are two ways to leave per-line comments when reviewing a pull request:

  • opening up each commit in a separate tab and going through them one-by-one (commits are listed in both the Discussion and the Commits tabs)

  • going to the Files Changed tab, and seeing the entire final result

I used to open up each commit individually, but:

  • if the person is forced to rebase (either to get rid of extra commits, or to clean up their history), the discussion is lost

  • under the discussions tab, there's no context, just the comment, which makes it possible to know that there's activity going on, but very difficult to keep track of exact context

  • it's possible that something you reviewed in one commit was fixed later on in the commit series, invalidating your comment

  • in the email notification, the "view it on github" link shows you the commit, not the associated pull request (and there's no easy way to find which pull request a commit is on, given just the commit)

I'd been ignoring the Files Changed tab for some reason, but I've now found that they actually don't have any the disadvantages of the per-commit view:

  • if the person is forced to rebase, old discussion is preserved in the pull request. If the rebase altered the diff, you'll get an "comment made on outdated diff"; if the diff is the same after a rebase, it shows up like it did before

  • under the discussions tab, it shows the context of the diff you're commenting on, along with your comment, which makes it very easy to keep track of the exact reason a comment was made (especially on short comments hehe)

  • you see the final version of the code that's being submitted

  • in the email notification, "view it on github" links to the pull request where all the discussion is taking place

All this is to say that if you're poking around and want to comment on something you see in someone else's pull request (and we do encourage code review in this manner!), use the Files Changed tab. It's much more useful.

Some examples:

  • actual pull request where you can see how easy it is to tell that a review comment has been handled (and what still needs work, or what doesn't need to be changed after all)

  • test pull request where I committed a change, commented using both the commit/files changed view, rebased+force pushed the rebase (which blew away the comment on the commit, but retained the comment on files changed), and then made the diff outdated -- which collapsed the discussion but still kept it visible

swaldman: A cute fluffy sheep curled up dreaming of Dreamwidth. Labelled "Simon: Bodger". (dw-dev)
[personal profile] swaldman
The new feature that I've been working on (tweeting about new entries) is ready for initial code review. However, it is definitely not ready to be landed into develop yet. How should I request this review?

I haven't created a pull request, because it's not ready to land.
I can't flag it as needs-review on bugzilla, because there is no attachment - I could put it into an attachment, but I imagine that commenting on the commit on github is easier for all concerned...

Any ideas or preferences? :-)
sophie: A cartoon-like representation of a girl standing on a hill, with brown hair, blue eyes, a flowery top, and blue skirt. ☀ (Default)
[personal profile] sophie
I've just put a mass user-creation script on the wiki. It allows you to create any number of users and have them be auto-validated. It will also randomly create watch/trust edges between the created users, and if you specify it, they will all join any given community. This makes it extremely useful when you have a bug that requires a large number of users to test, such as bug 2215: member list page: "jump to account" does not jump to username, which requires more than 100 users in a single community.

(I actually already had this script created some time ago, but hadn't released it publicly. However, when I saw [personal profile] kjwcode manually creating 100 users in order to test the patch on the aforementioned bug, I added the community-joining ability and offered the script to him, and realised I should probably release it for all to use!)

There are a few things the script doesn't do yet. For example, while it can make all the users join a community, it won't make any of them subscribe to it. If anybody wants more features added, let me know!

And, in the interests of code preservation (in case the wiki ever disappears for some reason - not that it should), I'm reproducing the code as it is now inside a cut. Bear in mind that it's possible that the version on the wiki may be more up-to-date if it's been some time since this post was made, so you should probably check on the wiki first!

Source code follows! )
denise: Image: Me, facing away from camera, on top of the Castel Sant'Angelo in Rome (Default)
[staff profile] denise
So, right now our major bottleneck in the dev process is code review! We're doing really well with patches being coded, and really well with bringing in new developers and getting them interested and working on things, but we've been having real problems finding people to do code review, and almost all the time it winds up being the people who are committing patches who do all the reviews.

This is a bad thing, for a few reasons: it means the committers burn out more quickly; everyone has different review styles, and notices different things, so having only one or two people doing the bulk of reviews leads to more things being missed; it slows down the turnaround time on commits; it means the committers are mostly working on reviewing and committing so they don't get as much time to code themselves; it means the committers feel rushed and pressured to get through the queue, because they know that nobody else is doing it, and that doesn't make for good work; and it means the rest of the dev team is losing out on the chance to learn from the process of reviewing someone else's code. I'd really like to see more peer review going on, with people reviewing each other, so that the committers have more of a chance to commit things that have already been reviewed by others instead of doing the bulk of the review themselves.

Ideally, I think we should be at around 60-70% peer review, with the remaining bits being committer review, and right now we're pretty much at 0%.

There are a few things I'd like to try to encourage people to do more peer reviewing, but before I do any of that, I figured I'd ask you guys some questions about doing reviews. Behind the cut is a poll, and I'd also love to hear any of your thoughts in comments. Poll answers are viewable in specific to nobody but maintainers of the comm, and if you have a comment that you'd like to leave, but don't feel comfortable doing so logged-in, anon commenting is enabled.

Read more... )
fu: Close-up of Fu, bringing a scoop of water to her mouth (Default)
[personal profile] fu
We've been seeing a lot of patches submitted lately, which is awesome (June hackathon, ILU ♥). So, now seems a perfect time to talk about reviews :D

Below the cut: outlining the roles of a dev, a reviewer, and a committer )

informal comments, full reviews, and what you should look for when looking over a patch )

ETA: Also of interest is [personal profile] yvi's entry Review 101 which explains how to do reviews in more detail, and also gives examples (thanks [personal profile] cesy for reminding me!)
kareila: "PERL!" (perl)
[personal profile] kareila
My initial revision of Dev Reviewing Guidelines is up on the wiki. Check it over and see if it looks good.

Please consider being a code reviewer! It's way easier to test code than it is to write it, but for some reason we always seem to have a backlog of unreviewed patches sitting around. If patches go through peer review, that reduces the workload of the handful of developers with commit access.
foxfirefey: Fox stealing an egg. (Default)
[personal profile] foxfirefey
Soooo tonight [staff profile] denise mentioned that [staff profile] mark makes a sad face every time he comes home all ready and rarin' to code only to find a gazillion patches to review. In order to try and spread this burden around, I've attempted to write a guide on the Code Review process so that more people can help with this important chore. I'm posting it for review and discussion, to make sure the instructions in it are sound.


dw_dev: The word "develop" using the Swirly D logo.  (Default)
Dreamwidth Open Source Development

April 2019



RSS Atom

Most Popular Tags

Style Credit

Expand Cut Tags

No cut tags
Page generated Apr. 24th, 2019 06:59 am
Powered by Dreamwidth Studios