Code review - Issue 7429044: Route namespace property stays in routehttps://codereview.appspot.com/2013-02-28T20:16:26+00:00rietveld
Message from unknown
2013-02-28T14:50:32+00:00jeff.pihachurn:md5:81a4115dcae5cf5661ecb4acebf72cc0
Message from jeff.pihach@canonical.com
2013-02-28T14:50:35+00:00jeff.pihachurn:md5:9d5c04d11069b369e1ab9a1f2344fe40
Please take a look.
Message from bcsaller@gmail.com
2013-02-28T15:27:15+00:00bcsallerurn:md5:e341be20b8ed718f118232b5bd6739a3
LGTM
But to be fair I don't know if I can review this as I gave you the original patch. I suppose we could call it pair programming though and still say it only needs one more pair of eyes.
Message from gary.poster@canonical.com
2013-02-28T16:01:31+00:00gary.posterurn:md5:a1c26e77fd4a0cc05017a3bf56e01c2c
LGTM with trivials.
Thanks!
Gary
https://codereview.appspot.com/7429044/diff/1/app/app.js
File app/app.js (right):
https://codereview.appspot.com/7429044/diff/1/app/app.js#newcode1077
app/app.js:1077: * default interface which allows for *callbacks to
I'm having trouble making sense of the second sentence. Is this the intent? If so, do you like it any better?
Override the App route builder. This method adds the ability to send multiple callbacks, and the ability to specify arbitrary additional attributes in the options argument.
https://codereview.appspot.com/7429044/diff/1/app/app.js#newcode1083
app/app.js:1083: callbacks = Y.Array.flatten(callbacks);
Do we really need to flatten, or is this just a spelling to create a new array? If the latter, how about just Y.Array(callbacks)? If the former, please add a comment explaining the purpose.
https://codereview.appspot.com/7429044/diff/1/app/app.js#newcode1092
app/app.js:1092: callback: callbacks[0]
What's the purpose of this backwards compatibility? If there's any chance that it would actually be used by something, perhaps this should be an anonymous function that executes all the callbacks? If it won't be used, why do we have it?
Message from bcsaller@gmail.com
2013-02-28T16:21:00+00:00bcsallerurn:md5:acc9d87efb734928a0f73e83413ceec2
https://codereview.appspot.com/7429044/diff/1/app/app.js
File app/app.js (right):
https://codereview.appspot.com/7429044/diff/1/app/app.js#newcode1083
app/app.js:1083: callbacks = Y.Array.flatten(callbacks);
On 2013/02/28 16:01:31, gary.poster wrote:
> Do we really need to flatten, or is this just a spelling to create a new array?
> If the latter, how about just Y.Array(callbacks)? If the former, please add a
> comment explaining the purpose.
Both of these points relate to how the code was ported from Y.Router. Previously we could call the superclass directly and then in the append only fashion of how routes worked make modifications to the last element, mixing in our extended attributes.
To allow for the mid-list insertion of the subapp code I ported that method up to App. Here, rather than prune options I directly mix our options in removing any ordering concerns.
`callback` was the old name for this, the change to `callbacks` happened in recent releases. I didn't any YUI code paths didn't depend on the old name, our did but I've since updated this with the changes to _dispatch.
Because `callbacks` can be a sequence flatten is used in their code but given that I can't support the *args signature they want (and still take options) we don't need the flatten, you're right it can become Array.
Message from jeff.pihach@canonical.com
2013-02-28T20:10:09+00:00jeff.pihachurn:md5:0f2b3e1425e3ed054c0ea807c2416349
Thanks for the reviews! See the comments below.
https://codereview.appspot.com/7429044/diff/1/app/app.js
File app/app.js (right):
https://codereview.appspot.com/7429044/diff/1/app/app.js#newcode1077
app/app.js:1077: * default interface which allows for *callbacks to
On 2013/02/28 16:01:31, gary.poster wrote:
> I'm having trouble making sense of the second sentence. Is this the intent? If
> so, do you like it any better?
>
> Override the App route builder. This method adds the ability to send multiple
> callbacks, and the ability to specify arbitrary additional attributes in the
> options argument.
Yeah I think that's a pretty good explanation. The native code allows you to supply either a `callback` or `callbacks`
https://codereview.appspot.com/7429044/diff/1/app/app.js#newcode1083
app/app.js:1083: callbacks = Y.Array.flatten(callbacks);
On 2013/02/28 16:21:00, bcsaller wrote:
> On 2013/02/28 16:01:31, gary.poster wrote:
> > Do we really need to flatten, or is this just a spelling to create a new
> array?
> > If the latter, how about just Y.Array(callbacks)? If the former, please add a
> > comment explaining the purpose.
>
> Both of these points relate to how the code was ported from Y.Router. Previously
> we could call the superclass directly and then in the append only fashion of how
> routes worked make modifications to the last element, mixing in our extended
> attributes.
>
> To allow for the mid-list insertion of the subapp code I ported that method up
> to App. Here, rather than prune options I directly mix our options in removing
> any ordering concerns.
>
> `callback` was the old name for this, the change to `callbacks` happened in
> recent releases. I didn't any YUI code paths didn't depend on the old name, our
> did but I've since updated this with the changes to _dispatch.
>
> Because `callbacks` can be a sequence flatten is used in their code but given
> that I can't support the *args signature they want (and still take options) we
> don't need the flatten, you're right it can become Array.
It's been changed to Y.Array(callbacks) and all of the tests still pass and functionally it appears to work as expected.
https://codereview.appspot.com/7429044/diff/1/app/app.js#newcode1092
app/app.js:1092: callback: callbacks[0]
On 2013/02/28 16:01:31, gary.poster wrote:
> What's the purpose of this backwards compatibility? If there's any chance that
> it would actually be used by something, perhaps this should be an anonymous
> function that executes all the callbacks? If it won't be used, why do we have
> it?
Although we are pretty certain it is no longer required I would like to leave it in now until this is split into an extension and can be properly tested.
Message from unknown
2013-02-28T20:12:52+00:00jeff.pihachurn:md5:26a092150a9126ced74486d256deae0d
Message from jeff.pihach@canonical.com
2013-02-28T20:16:26+00:00jeff.pihachurn:md5:1932782ddf9e17c2d81825b74d4e8f57
*** Submitted:
Route namespace property stays in route
R=bcsaller, gary.poster
CC=
https://codereview.appspot.com/7429044