Just a friendly reminder that those of us with commit access to Phobos should be reviewing pull requests at least periodically. All pull requests are supposed to be reviewed by at least one Phobos dev (and preferrably at least one Phobos dev who didn't actually create the request).

I know that we're all busy (and some of us are extremely busy), but if no one is reviewing pull requests and merging them, then pull requests will just sit there and good work will be wasted (and those who submit them may be discouraged about making further submissions). So, please try and review pull requests at least once in a while, if not regularly.

- Jonathan M Davis
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

One issue I'm having with reviewing: I can't seem to find a place to see the latest and greatest proposed code diff'ed against the extant code. Is there a possibility to do that? I can only see stuff changed by each individual diff, which is fine if I look for a specific change, but what I really want to review is everything proposed against everything current.

Thanks,

Andrei

On 7/1/12 9:35 PM, Jonathan M Davis wrote:
> Just a friendly reminder that those of us with commit access to Phobos should
> be reviewing pull requests at least periodically. All pull requests are
> supposed to be reviewed by at least one Phobos dev (and preferrably at least
> one Phobos dev who didn't actually create the request).
>
> I know that we're all busy (and some of us are extremely busy), but if no one
> is reviewing pull requests and merging them, then pull requests will just sit
> there and good work will be wasted (and those who submit them may be
> discouraged about making further submissions). So, please try and review pull
> requests at least once in a while, if not regularly.
>
> - Jonathan M Davis
> _______________________________________________
> phobos mailing list
> phobos@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

On Sunday, July 01, 2012 22:18:44 Andrei Alexandrescu wrote:
> One issue I'm having with reviewing: I can't seem to find a place to see
> the latest and greatest proposed code diff'ed against the extant code.
> Is there a possibility to do that? I can only see stuff changed by each
> individual diff, which is fine if I look for a specific change, but what
> I really want to review is everything proposed against everything current.

Well, the diff that you see when you click on diff in the pull request gives a diff in comparison to the commits prior to the changes being committed.  So, what you're looking for is being able to diff the state of all of Phobos in the pull request against the state of all of Phobos in the current master?

I believe that this explains how to do it:

https://github.com/blog/683-cross-repository-compare-view

Though I don't see how to actually _get_ to the compare view that they're describing without typing in the URL. For Kenji's repository, you'd type

https://github.com/9rnsr/phobos/compare/master

For mine you'd type

https://github.com/jmdavis/phobos/compare/master

and then you can manipulate the repository and branches being compared using the compare view.

Personally, I think that seeing all of that extra information is just distracting though.

- Jonathan M Davis
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

On 7/1/12 11:30 PM, Jonathan M Davis wrote:
> Well, the diff that you see when you click on diff in the pull request gives a
> diff in comparison to the commits prior to the changes being committed.  So,
> what you're looking for is being able to diff the state of all of Phobos in the
> pull request against the state of all of Phobos in the current master?

Yes. Say I enter a pull request that has been already discussed and modified as a result of the discussion. From here there are two paths of analyzing the request:

1. Going through all comments and reactions to them, keeping a mental track of the evolution of the code;

2. Reading the comments only for validity and then look at the "integral" - the pull request as it is being proposed right now, against the existing code.

I very much prefer (2) perhaps because phabricator.org used me to it. In unusually large pull requests I might occasionally care for how the proponent responded to a particular comment. But generally I want to see what's there now and what's being proposed, wholesale.

> I believe that this explains how to do it:
>
> https://github.com/blog/683-cross-repository-compare-view
>
> Though I don't see how to actually _get_ to the compare view that they're
> describing without typing in the URL. For Kenji's repository, you'd type
>
> https://github.com/9rnsr/phobos/compare/master
>
> For mine you'd type
>
> https://github.com/jmdavis/phobos/compare/master
>
> and then you can manipulate the repository and branches being compared using
> the compare view.
>
> Personally, I think that seeing all of that extra information is just
> distracting though.

I don't see that information as not extra, but instead exactly what the code will be after the merge.

I think I found what I want. At the bottom of the request there's this: "Tip: You can also add notes to lines changed in a file under Diff". The word "Diff" is linked, and the link seems to point to what I need.


Andrei

_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

On Monday, July 02, 2012 01:11:31 Andrei Alexandrescu wrote:
> On 7/1/12 11:30 PM, Jonathan M Davis wrote:
> > Well, the diff that you see when you click on diff in the pull request gives a diff in comparison to the commits prior to the changes being committed.  So, what you're looking for is being able to diff the state of all of Phobos in the pull request against the state of all of Phobos in the current master?
> Yes. Say I enter a pull request that has been already discussed and modified as a result of the discussion. From here there are two paths of analyzing the request:
> 
> 1. Going through all comments and reactions to them, keeping a mental track of the evolution of the code;
> 
> 2. Reading the comments only for validity and then look at the "integral" - the pull request as it is being proposed right now, against the existing code.
> 
> I very much prefer (2) perhaps because phabricator.org used me to it. In unusually large pull requests I might occasionally care for how the proponent responded to a particular comment. But generally I want to see what's there now and what's being proposed, wholesale.

Well, if the request is rebased on master, then that fixes the problem, but if you want to compare agains the current master, that's up to you.

> > I believe that this explains how to do it:
> > 
> > https://github.com/blog/683-cross-repository-compare-view
> > 
> > Though I don't see how to actually _get_ to the compare view that they're describing without typing in the URL. For Kenji's repository, you'd type
> > 
> > https://github.com/9rnsr/phobos/compare/master
> > 
> > For mine you'd type
> > 
> > https://github.com/jmdavis/phobos/compare/master
> > 
> > and then you can manipulate the repository and branches being compared using the compare view.
> > 
> > Personally, I think that seeing all of that extra information is just distracting though.
> 
> I don't see that information as not extra, but instead exactly what the code will be after the merge.

When it includes _all_ of the changes in Phobos since the brach was merged, that can be a lot of extraneous stuff. And the longer that a pull request has been around without merging in or rebasing on master, the worse that it gets. But if you don't mind, then that's fine. I just find it to be too much to look at personally.

> I think I found what I want. At the bottom of the request there's this: "Tip: You can also add notes to lines changed in a file under Diff". The word "Diff" is linked, and the link seems to point to what I need.

Cool. That'll make it considerably easier.

- Jonathan M Davis
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

On 2 Jul 2012, at 7:11, Andrei Alexandrescu wrote:
> I think I found what I want. At the bottom of the request there's this: "Tip: You can also add notes to lines changed in a file under Diff". The word "Diff" is linked, and the link seems to point to what I need.

You also get this view by clicking the Diff tab at the top (along Discussion and Commits). I agree that is incredibly useful – not sure how you »survived« so far… ;)

David
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

On Monday, July 02, 2012 09:04:14 David Nadlinger wrote:
> On 2 Jul 2012, at 7:11, Andrei Alexandrescu wrote:
> > I think I found what I want. At the bottom of the request there's this: "Tip: You can also add notes to lines changed in a file under Diff". The word "Diff" is linked, and the link seems to point to what I need.
> 
> You also get this view by clicking the Diff tab at the top (along Discussion and Commits). I agree that is incredibly useful – not sure how you »survived« so far… ;)

That's different though. That shows the diff between the commit before the pull request and the last commit of the pull request. What Andrei seems to want is the diff between the last commit of the pull request and the latest commit in Phobos, which is very different, especially if the pull request has been around for a while and a lot of other stuff has been committed.

- Jonathan M Davis
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

On 2 Jul 2012, at 9:22, Jonathan M Davis wrote:
> On Monday, July 02, 2012 09:04:14 David Nadlinger wrote:
>> On 2 Jul 2012, at 7:11, Andrei Alexandrescu wrote:
>> You also get this view by clicking the Diff tab at the top (along
>> Discussion and Commits). I agree that is incredibly useful – not sure
>> how you »survived« so far… ;)
>
> That's different though. That shows the diff between the commit before the pull
> request and the last commit of the pull request. What Andrei seems to want is
> the diff between the last commit of the pull request and the latest commit in
> Phobos, which is very different, especially if the pull request has been around
> for a while and a lot of other stuff has been committed.

Ah, I see – but the link Andrei mentioned seems to point to exactly the same address as the "tab" at the top.

David
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

On Monday, July 02, 2012 10:40:06 David Nadlinger wrote:
> On 2 Jul 2012, at 9:22, Jonathan M Davis wrote:
> > On Monday, July 02, 2012 09:04:14 David Nadlinger wrote:
> >> On 2 Jul 2012, at 7:11, Andrei Alexandrescu wrote:
> >> You also get this view by clicking the Diff tab at the top (along
> >> Discussion and Commits). I agree that is incredibly useful – not
> >> sure
> >> how you »survived« so far… ;)
> > 
> > That's different though. That shows the diff between the commit before
> > the pull
> > request and the last commit of the pull request. What Andrei seems to
> > want is
> > the diff between the last commit of the pull request and the latest
> > commit in
> > Phobos, which is very different, especially if the pull request has
> > been around
> > for a while and a lot of other stuff has been committed.
> 
> Ah, I see – but the link Andrei mentioned seems to point to exactly the same address as the "tab" at the top.

If that's the case, then it won't help him any.

Yeah, it looks like it just takes you to the diff tab. I don't know of an easy way to get to the comparison tool that I mentioned earlier. If you type in the URL directly, it works, but I don't see any easy way to click to it. Maybe it would be clearer if I read all of the article that I linked to, but it seemed to assume that you already knew how to get there.

- Jonathan M Davis
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

On 7/2/12 3:22 AM, Jonathan M Davis wrote:
> On Monday, July 02, 2012 09:04:14 David Nadlinger wrote:
>> On 2 Jul 2012, at 7:11, Andrei Alexandrescu wrote:
>>> I think I found what I want. At the bottom of the request there's
>>> this: "Tip: You can also add notes to lines changed in a file under
>>> Diff". The word "Diff" is linked, and the link seems to point to what
>>> I need.
>>
>> You also get this view by clicking the Diff tab at the top (along
>> Discussion and Commits). I agree that is incredibly useful – not sure
>> how you »survived« so far… ;)
>
> That's different though. That shows the diff between the commit before the pull
> request and the last commit of the pull request. What Andrei seems to want is
> the diff between the last commit of the pull request and the latest commit in
> Phobos, which is very different, especially if the pull request has been around
> for a while and a lot of other stuff has been committed.

I think it's still an improvement. Thanks David.

Andrei
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos