Issue 7381055: Created app subapp extension
DescriptionCreated app subapp extension
https://code.launchpad.net/~hatch/juju-gui/1130787-subapp-app-extension/+merge/150414
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 29
Patch Set 2 : Created app subapp extension #
Total comments: 10
Patch Set 3 : Created app subapp extension #Patch Set 4 : Created app subapp extension #Patch Set 5 : Created app subapp extension #
Total comments: 22
Patch Set 6 : Created app subapp extension #
Total comments: 2
Patch Set 7 : Created app subapp extension #Patch Set 8 : Created app subapp extension #Patch Set 9 : Created app subapp extension #Patch Set 10 : Created app subapp extension #Patch Set 11 : Created app subapp extension #MessagesTotal messages: 21
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Thanks for the branch. I have some minor comments below but there are still things I'm not clear about here. At minimum it seems that the parent should be the eventTarget of the subApp when its created? Is that going to live in another branch? I suppose this branch presents too small view of the final picture for me to understand how this plays out in reality. Also is there a use case for subapp removal? I think we can pass on that for now as well, but we might note this somewhere. Looking forward to seeing more. https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... File app/assets/javascripts/app-subapp-extension.js (right): https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:9: value: {} As we don't currently support removal should this be writeOnce and/or initOnly? https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:21: config: {} type -> instance, its more clear and follow the App.views.viewName.instance model. this.get('subApps'), this.subApplications and params named subApps all occur below making things a little harder to follow than they need to be. https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:56: this._augmentParentRoutes(routes); parent seems unnecessary in the naming, _this_ is the parent. _augmentRoutes or _mixinRoutes? https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:68: @param {array} subApps an array of sub abb objects and configs. s/abb/app https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:71: for (var i = 0; i < subApps.length; i += 1) { Y.each? We usually favor iterators to native loops.
Sign in to reply to this message.
Thanks for the review - I'll make the changes and resubmit https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... File app/assets/javascripts/app-subapp-extension.js (right): https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:9: value: {} On 2013/02/25 20:12:13, bcsaller wrote: > As we don't currently support removal should this be writeOnce and/or initOnly? I understand where you're coming from but I'm not really a fan of adding that restriction when it doesn't gain us anything - possibly something to investigate in the future. https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:21: config: {} On 2013/02/25 20:12:13, bcsaller wrote: > type -> instance, its more clear and follow the App.views.viewName.instance Sorry yes this documentation is incorrect I will fix The new syntax mirrors how Y.App does views { type: 'name', ... } > model. > > this.get('subApps'), this.subApplications and params named subApps all occur > below making things a little harder to follow than they need to be. I'll evaluate a better naming convention as your right this could likely cause name clashes. https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:56: this._augmentParentRoutes(routes); On 2013/02/25 20:12:13, bcsaller wrote: > parent seems unnecessary in the naming, _this_ is the parent. > > _augmentRoutes or _mixinRoutes? Agreed, will fix https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:68: @param {array} subApps an array of sub abb objects and configs. On 2013/02/25 20:12:13, bcsaller wrote: > s/abb/app will fix https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-s... app/assets/javascripts/app-subapp-extension.js:71: for (var i = 0; i < subApps.length; i += 1) { On 2013/02/25 20:12:13, bcsaller wrote: > Y.each? We usually favor iterators to native loops. Agreed, will fix
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Hi Jeff. I like where this is going. As I said on IRC, my biggest concern is a lack of tests. I have some small-to-medium sized issues here as well. Notice that, because I sat on this review for awhile, the notes are from an earlier revision. Hopefully they are still mostly worthwhile. Thanks! Gary https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... File app/assets/javascripts/app-subapp-extension.js (right): https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:1: 'use strict'; Please add the yuidoc @module stuff here (see the topology example, for instance) https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:5: function SubAppRegistration() {} Please add a @class directive like the topology example one. Note that make view-docs is a nice way to double-check that you have made the docs you want. https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:25: @type {array} We usually comment these with "*" marks before each line. If YUI doc does not require this, as far as I am concerned, rejoice!!! And share the knowledge with the rest of us. But if the doc parser is unhappy, try adding those. https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:39: Adds the sub application and it's routes to the parent application. typo: its https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:42: @param {string} subApp string referance to an instantiable Y.App object. typo: reference I don't understand this comment. The string names a property of the parent Y.App object that is the factory for the SubApp? No, that's not it. You get it off of Y? Is this common? Never seen that before. If I understand correctly that you are supposed to create an application object hanging off of the Y (framework) object, I don't really like that on the face of it, but I'll defer to you if you say that you are confident that this is the YUI way. We have instead made our own top-level objects, rather than polluting the framework namespace. That seems safer to me. https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:77: Public method to refresh routes from the sub apps. When would you need to call this? https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:80: @param {integer} index index of the subapp to refresh if undefined typo: s/index index/index/ ...to refresh. If undefined,... https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:102: case 'number': Is there really a use case for this? Seems like a YAGNI. If we have a use case, never mind https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:109: case 'undefined': Sometimes we have a default case that throws an error because of unexpected input. I like that approach and suggest it, but do not require it. https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:112: subRoutes = subApps[i].getSubAppRoutes(); Is getSubAppRoutes the thing that is responsible for translating the route callables into being run in the context of the subapp? https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:134: if (value.path !== '*') { hm. I guess this is a reasonable definition of middleware. It makes me a bit nervous that there will be conditional middleware of some sort, only applied to certain paths...
Sign in to reply to this message.
Thanks a bunch for your comments I'll get on them right away! I only have one follow-up for line 109 so if you could let me know if that was your expectation that would be great! Thanks again https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... File app/assets/javascripts/app-subapp-extension.js (right): https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:1: 'use strict'; On 2013/02/25 21:20:05, gary.poster wrote: > Please add the yuidoc @module stuff here (see the topology example, for > instance) will fix https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:5: function SubAppRegistration() {} On 2013/02/25 21:20:05, gary.poster wrote: > Please add a @class directive like the topology example one. > > Note that make view-docs is a nice way to double-check that you have made the > docs you want. will fix https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:25: @type {array} On 2013/02/25 21:20:05, gary.poster wrote: > We usually comment these with "*" marks before each line. If YUI doc does not > require this, as far as I am concerned, rejoice!!! And share the knowledge with > the rest of us. But if the doc parser is unhappy, try adding those. YUIdoc doesn't require them it simply requires two stars after the slash /** docs */ but I may be using a newer version - I'll re-check once I fix the above missing docs https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:39: Adds the sub application and it's routes to the parent application. On 2013/02/25 21:20:05, gary.poster wrote: > typo: its will fix https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:42: @param {string} subApp string referance to an instantiable Y.App object. On 2013/02/25 21:20:05, gary.poster wrote: > typo: reference > > I don't understand this comment. The string names a property of the parent > Y.App object that is the factory for the SubApp? No, that's not it. You get it > off of Y? Is this common? Never seen that before. If I understand correctly > that you are supposed to create an application object hanging off of the Y > (framework) object, I don't really like that on the face of it, but I'll defer > to you if you say that you are confident that this is the YUI way. We have > instead made our own top-level objects, rather than polluting the framework > namespace. That seems safer to me. My thought process behind this was that I wanted to mirror the syntax of how Y.App deals with views with the syntax { type: 'path.to.view' } to make it more consistent. In looking at how the codebase does modules they are almost always hung off of the Y instance under their own namespace so this allows them to keep with the same syntax ie) 'juju.subapps.mysubapp' https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:77: Public method to refresh routes from the sub apps. On 2013/02/25 21:20:05, gary.poster wrote: > When would you need to call this? If you manipulated the routes on a subapp after instantiation you would need to push those back up to the parent. But in all honesty I I created it while prototyping and just left it in :-) https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:80: @param {integer} index index of the subapp to refresh if undefined On 2013/02/25 21:20:05, gary.poster wrote: > typo: s/index index/index/ > > ...to refresh. If undefined,... will fix https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:102: case 'number': On 2013/02/25 21:20:05, gary.poster wrote: > Is there really a use case for this? Seems like a YAGNI. If we have a use > case, never mind The idea behind allowing the dev to specify a number was a foreshadow into future platform differences - they may not want to extract all routes on a mobile device for example https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:109: case 'undefined': On 2013/02/25 21:20:05, gary.poster wrote: > Sometimes we have a default case that throws an error because of unexpected > input. I like that approach and suggest it, but do not require it. Sorry I don't follow - are you saying that we should have an explicit option to load all instead of defaulting to it? https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:112: subRoutes = subApps[i].getSubAppRoutes(); On 2013/02/25 21:20:05, gary.poster wrote: > Is getSubAppRoutes the thing that is responsible for translating the route > callables into being run in the context of the subapp? yes getSubAppRoutes fetches its routes and adds its namespace to the routes. I did this because I didn't want to clobber the native router getter. https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:134: if (value.path !== '*') { On 2013/02/25 21:20:05, gary.poster wrote: > hm. I guess this is a reasonable definition of middleware. It makes me a bit > nervous that there will be conditional middleware of some sort, only applied to > certain paths... Maybe I should comment this further - what it's doing is grabbing the parent routes and adding any child routes after the 'middleware' which is really anything in the parent route with a * path
Sign in to reply to this message.
Hey Jeff. I made replies to some of your comments. https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... File app/assets/javascripts/app-subapp-extension.js (right): https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:42: @param {string} subApp string referance to an instantiable Y.App object. On 2013/02/25 21:46:59, jeff.pihach wrote: > On 2013/02/25 21:20:05, gary.poster wrote: > > typo: reference > > > > I don't understand this comment. The string names a property of the parent > > Y.App object that is the factory for the SubApp? No, that's not it. You get > it > > off of Y? Is this common? Never seen that before. If I understand correctly > > that you are supposed to create an application object hanging off of the Y > > (framework) object, I don't really like that on the face of it, but I'll defer > > to you if you say that you are confident that this is the YUI way. We have > > instead made our own top-level objects, rather than polluting the framework > > namespace. That seems safer to me. > My thought process behind this was that I wanted to mirror the syntax of how > Y.App deals with views with the syntax { type: 'path.to.view' } to make it more > consistent. > > In looking at how the codebase does modules they are almost always hung off of > the Y instance under their own namespace so this allows them to keep with the > same syntax ie) 'juju.subapps.mysubapp' Ah I thought juju was not off of Y. All's well then, thanks. https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:77: Public method to refresh routes from the sub apps. On 2013/02/25 21:46:59, jeff.pihach wrote: > On 2013/02/25 21:20:05, gary.poster wrote: > > When would you need to call this? > If you manipulated the routes on a subapp after instantiation you would need to > push those back up to the parent. > But in all honesty I I created it while prototyping and just left it in :-) Heh, cool. I suggest removing it then. When you write tests, and have to decide between testing this or removing it, I bet you will prefer removing it too. :-) https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:102: case 'number': On 2013/02/25 21:46:59, jeff.pihach wrote: > On 2013/02/25 21:20:05, gary.poster wrote: > > Is there really a use case for this? Seems like a YAGNI. If we have a use > > case, never mind > The idea behind allowing the dev to specify a number was a foreshadow into > future platform differences - they may not want to extract all routes on a > mobile device for example Huh, ok. I suggest either giving a believable example of how this would work in a comment, or deleting it as a YAGNI for now. It looks easy to add back it. https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:109: case 'undefined': On 2013/02/25 21:46:59, jeff.pihach wrote: > On 2013/02/25 21:20:05, gary.poster wrote: > > Sometimes we have a default case that throws an error because of unexpected > > input. I like that approach and suggest it, but do not require it. > Sorry I don't follow - are you saying that we should have an explicit option to > load all instead of defaulting to it? > No, just a "default:" that throws an error https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:112: subRoutes = subApps[i].getSubAppRoutes(); On 2013/02/25 21:46:59, jeff.pihach wrote: > On 2013/02/25 21:20:05, gary.poster wrote: > > Is getSubAppRoutes the thing that is responsible for translating the route > > callables into being run in the context of the subapp? > yes getSubAppRoutes fetches its routes and adds its namespace to the routes. I > did this because I didn't want to clobber the native router getter. +1 on not clobbering. I was talking about having named callables in the routes resolved off of the subapp, and with a "this" that is the subapp, though. Can we still do that? https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:134: if (value.path !== '*') { On 2013/02/25 21:46:59, jeff.pihach wrote: > On 2013/02/25 21:20:05, gary.poster wrote: > > hm. I guess this is a reasonable definition of middleware. It makes me a bit > > nervous that there will be conditional middleware of some sort, only applied > to > > certain paths... > Maybe I should comment this further - what it's doing is grabbing the parent > routes and adding any child routes after the 'middleware' which is really > anything in the parent route with a * path Yeah, I understood most of that. That's not a robust definition of middleware though. Middleware is really something that passes through to more routes, whether the path is * or /foo/* or even simply /foo. This code assumes the common case, and slams over less common cases. It makes me nervous that it will be a source for rare but occasional surprising bugs.
Sign in to reply to this message.
https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... File app/assets/javascripts/app-subapp-extension.js (right): https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-suba... app/assets/javascripts/app-subapp-extension.js:134: if (value.path !== '*') { On 2013/02/25 21:59:00, gary.poster wrote: > On 2013/02/25 21:46:59, jeff.pihach wrote: > > On 2013/02/25 21:20:05, gary.poster wrote: > > > hm. I guess this is a reasonable definition of middleware. It makes me a > bit > > > nervous that there will be conditional middleware of some sort, only applied > > to > > > certain paths... > > Maybe I should comment this further - what it's doing is grabbing the parent > > routes and adding any child routes after the 'middleware' which is really > > anything in the parent route with a * path > Yeah, I understood most of that. That's not a robust definition of middleware > though. Middleware is really something that passes through to more routes, > whether the path is * or /foo/* or even simply /foo. This code assumes the > common case, and slams over less common cases. It makes me nervous that it will > be a source for rare but occasional surprising bugs. While native Y.App.route doesn't support annotations we do, we could simply tag routes as `middleware`: true. Another option is simply to append namespaced routes at the end. Now that we support qualified matches (requiring the namespace attr to match) there is no need to place them anywhere but the end. The issue before was that a default route at the end would match anything preventing namespaced routes from being matched. Now the default route would be passed over. Unless special provision is taken such that namespace attrs on subapp routes were preserved (rather than set by the binding to App) this policy would work.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Thanks for the continued work on this branch. Personally I think we should switch to a namespaced, append routes model, that would remove the need to do the placement test that I suggest below. I think gary would agree that this is simpler and more foolproof. https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... File app/assets/javascripts/app-subapp-extension.js (right): https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:110: } No default still? https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:126: middlewareIndex, groupedRoutes; groupedRoutes is unused. https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens... File test/test_subapp_app_extension.js (right): https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens... test/test_subapp_app_extension.js:68: 'Number of routes does not match'); With a change to middleware handling we should test that insertion happens in the currently expected place.
Sign in to reply to this message.
LGTM with trivials.
Hi Jeff. I have a lot of comments and suggestions and thoughts. The only thing
that's a clear "please do this" request is the trivial grammatical change
("it's" -> "its").
Everything else is thoughts. What you've done is good and has plenty of
precedent, but I think what we talked about would have been better. If you'd
like to leave it as it is, that's fine; or land it.
Thanks
Gary
https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-...
File app/assets/javascripts/app-subapp-extension.js (right):
https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-...
app/assets/javascripts/app-subapp-extension.js:47: Adds the sub application and
it's routes to the parent application.
trivial typo, but repeated from previous feedback: you mean "its."
As a matter of policy, we are supposed to go through every comment in a review
and acknowledge the comment, so the reviewer can see that their effort was
recognized, understood, and acted on. For some of us, that is one of those
rules "more honored in the breach" :-) but it really shouldn't be. Since you
missed this (trivial) comment, it might be a good habit to get into.
https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-...
app/assets/javascripts/app-subapp-extension.js:54: var SubAppObject =
Y.Object.getValue(Y, subApp.split('.')),
I personally think we ought to just pass in factories directly, not strings.
<shrug>
https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-...
app/assets/javascripts/app-subapp-extension.js:63:
That's a lot of whitespace! I think we have a "don't use lots of whitespace
unless you really really want to" kind of non-rule, but could be wrong. Do what
you want here.
https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-...
app/assets/javascripts/app-subapp-extension.js:64: this._augmentRoutes(routes);
The addSubApp function reads very nicely.
https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-...
app/assets/javascripts/app-subapp-extension.js:129: if (value.path !== '*') {
I still question this, but <shrug>
https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens...
File test/test_subapp_app_extension.js (right):
https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens...
test/test_subapp_app_extension.js:16: app = new juju.App();
For this level of tests, IMO it would have been nicer to use a generic App, with
your extension mixed in.
https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens...
test/test_subapp_app_extension.js:18: mocks = {
I thought this was explicitly what you were not going to do! I don't mind, but
IMO the plan we discussed, as driven by the concerns you expressed, was nicer.
https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens...
test/test_subapp_app_extension.js:55: assert.deepEqual(app._extractRoutes(),
mocks.subAppRoutes,
Since you are testing what you claim to be an implementation detail ("_*"), it
makes me wonder if you might have been happier with a (slightly?) different
factoring.
https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens...
test/test_subapp_app_extension.js:68: 'Number of routes does not match');
Another deepEqual, this time of the trailing routes, would have been cool. This
is alright though, given the previous test.
Sign in to reply to this message.
Thanks for the reviews! Please see the attached comments https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... File app/assets/javascripts/app-subapp-extension.js (right): https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:47: Adds the sub application and it's routes to the parent application. On 2013/02/26 22:07:25, gary.poster wrote: > trivial typo, but repeated from previous feedback: you mean "its." > > As a matter of policy, we are supposed to go through every comment in a review > and acknowledge the comment, so the reviewer can see that their effort was > recognized, understood, and acted on. For some of us, that is one of those > rules "more honored in the breach" :-) but it really shouldn't be. Since you > missed this (trivial) comment, it might be a good habit to get into. Fixed. https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:54: var SubAppObject = Y.Object.getValue(Y, subApp.split('.')), On 2013/02/26 22:07:25, gary.poster wrote: > I personally think we ought to just pass in factories directly, not strings. > <shrug> No problem - consider it changed. https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:63: On 2013/02/26 22:07:25, gary.poster wrote: > That's a lot of whitespace! I think we have a "don't use lots of whitespace > unless you really really want to" kind of non-rule, but could be wrong. Do what > you want here. I collapsed them into like commands to tighten it up a bit. https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:64: this._augmentRoutes(routes); On 2013/02/26 22:07:25, gary.poster wrote: > The addSubApp function reads very nicely. Thanks https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:110: } On 2013/02/26 22:00:56, bcsaller wrote: > No default still? Default has now been added. https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:126: middlewareIndex, groupedRoutes; On 2013/02/26 22:00:56, bcsaller wrote: > groupedRoutes is unused. Poof it's gone! https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:129: if (value.path !== '*') { On 2013/02/26 22:07:25, gary.poster wrote: > I still question this, but <shrug> I'm going to leave this insertion method in here for now - at least until we get the namespace code separated out and a functional sub app using the code - it's trivial to change but I know that this works for the prototype use case :-)
Sign in to reply to this message.
On 2013/02/27 17:04:29, jeff.pihach wrote: > Thanks for the reviews! Please see the attached comments :-) thanks, sounds good.
Sign in to reply to this message.
LGTM I think this is in shape to land so we can move forward with it. I still have some reservations about how it all fits together, but I'm convinced we will be able to make this work. Thanks again.
Sign in to reply to this message.
Hey sorry guys I forgot to 'save' my comments on the test page - please see those comments below. https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens... File test/test_subapp_app_extension.js (right): https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens... test/test_subapp_app_extension.js:16: app = new juju.App(); On 2013/02/26 22:07:25, gary.poster wrote: > For this level of tests, IMO it would have been nicer to use a generic App, with > your extension mixed in. Sure thing - no problem I can switch that over. https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens... test/test_subapp_app_extension.js:18: mocks = { On 2013/02/26 22:07:25, gary.poster wrote: > I thought this was explicitly what you were not going to do! I don't mind, but > IMO the plan we discussed, as driven by the concerns you expressed, was nicer. True however in practice it ended up being another level of abstraction which was only valuable in testing and created needless complexity everywhere else so I decided to bench those changes. https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extens... test/test_subapp_app_extension.js:68: 'Number of routes does not match'); On 2013/02/26 22:07:25, gary.poster wrote: > Another deepEqual, this time of the trailing routes, would have been cool. This > is alright though, given the previous test. Because I'm benching changing the insertion point of the routes I am going to leave this as is in conjunction with the above test. I foresee changes in it's future... also I can't deep equal here because the routes are augmented with quite a bit of meta data that I am unable to mock. I'll have to investigate a better way to do this once we have a fleshed out subapp
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
LGTM still--looks even better. :-) Thanks Gary https://codereview.appspot.com/7381055/diff/23001/app/assets/javascripts/app-... File app/assets/javascripts/app-subapp-extension.js (right): https://codereview.appspot.com/7381055/diff/23001/app/assets/javascripts/app-... app/assets/javascripts/app-subapp-extension.js:50: @param {string} SubApp string referance to an instantiable Y.App object. No longer a string reference https://codereview.appspot.com/7381055/diff/23001/test/test_subapp_app_extens... File test/test_subapp_app_extension.js (right): https://codereview.appspot.com/7381055/diff/23001/test/test_subapp_app_extens... test/test_subapp_app_extension.js:12: [Y.juju.SubAppRegistration], {}); Cool.
Sign in to reply to this message.
*** Submitted: Created app subapp extension R=bcsaller, gary.poster CC= https://codereview.appspot.com/7381055
Sign in to reply to this message.
|
