<div dir="ltr">On Wed, Jan 30, 2013 at 10:50 AM, Frank Shearar <span dir="ltr"><<a href="mailto:frank.shearar@gmail.com" target="_blank">frank.shearar@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 30 January 2013 18:33, <<a href="mailto:commits@source.squeak.org">commits@source.squeak.org</a>> wrote:<br>
> Frank Shearar uploaded a new version of Kernel to project The Inbox:<br>
> <a href="http://source.squeak.org/inbox/Kernel-fbs.735.mcz" target="_blank">http://source.squeak.org/inbox/Kernel-fbs.735.mcz</a><br>
><br>
> ==================== Summary ====================<br>
><br>
> Name: Kernel-fbs.735<br>
> Author: fbs<br>
> Time: 30 January 2013, 6:33:47.078 pm<br>
> UUID: 18ffff61-cfcf-4843-8c64-caea3ceb3fbc<br>
> Ancestors: Kernel-fbs.734<br>
><br>
> Actually, return a Message, and find out the receiver of the Message from the signalercontext.<br>
><br>
> Also, _returning_ the value of #subclassResponsibility means that you can return to the original caller the value of your just implemented method.<br>
><br>
> =============== Diff against Kernel-fbs.734 ===============<br>
><br>
> Item was added:<br>
> + ----- Method: ContextPart>>asMessage (in category 'converting') -----<br>
> + asMessage<br>
> + | sender selector args |<br>
> + sender := self sender.<br>
> + selector := sender method selector.<br>
> + args := Array new: selector numArgs.<br>
> + 1 to: selector numArgs do: [ :i | args at: i put: (sender tempAt: i)].<br>
> + ^ Message selector: selector arguments: args.!<br>
><br>
> Item was removed:<br>
> - ----- Method: ContextPart>>asMessageSend (in category 'converting') -----<br>
> - asMessageSend<br>
> - | sender selector args |<br>
> - sender := self sender.<br>
> - selector := sender method selector.<br>
> - args := Array new: selector numArgs.<br>
> - 1 to: selector numArgs do: [ :i | args at: i put: (sender tempAt: i)].<br>
> - ^ MessageSend receiver: self receiver selector: selector arguments: args.!<br>
<br>
</div></div><snip><br>
<br>
Diffs serve as a helper for reviewers. To that end, the diffs ought to<br>
actually show what changes would be applied to trunk should the change<br>
be accepted. This diff, for instance, shows the removal of<br>
#asMessageSend and the addition of #asMessage, but really the change<br>
applied to trunk will be just the addition of #asMessage.<br>
<br>
In other words when something undergoes a few rounds of review (and<br>
I'd think this should be the _norm_) the reviewer must reconstruct a<br>
series of diffs to get an idea of how trunk will change.<br>
<br>
Wouldn't it be better to diff against trunk rather than against the<br>
mcz's ancestor? (*)<br>
<br>
frank<br></blockquote><div style>Yes, it would be better to diff against trunk. But 'Trunk' isn't a defined state - what you have currently as Trunk isn't what I have currently as trunk - I haven't updated as recently as you have, for instance, so the changes that will be applied if I accepted/updated will be different than if you accept/update.</div>
<div style><br></div><div style>Maybe you are talking about diffs against the most recent predecessor in the Trunk Repository? That should be doable - but if there is no predecessor (a completely new package is moved in), then all of the messages are new.</div>
<div style><br></div><div style>-Chris</div></div></div></div>