Explanation and cleanups for tardis URL code

Explanation and cleanups for tardis URL code

This branch is largely comments, plus a single cleanup of the "routes seen per URL" mechanism that I think is easier to understand with no downsides.

https://code.launchpad.net/~gary/juju-gui/tardis-exegesis/+merge/149740

(do not edit description out of merge proposal)

[gary.poster, 2013-02-21 02:47:18]

Please take a look.

[bcsaller, 2013-02-21 04:38:46]

Thanks for thinking more deeply about this code. Happy to talk about this more
and I hope the inline comments are helpful.

https://codereview.appspot.com/7372044/diff/1/app/app.js
File app/app.js (left):

https://codereview.appspot.com/7372044/diff/1/app/app.js#oldcode405
app/app.js:405: self._dispatching = self._dispatched = true;
This code is largely mapped from Y.Router, some of the base class code depends
in part on these flags so I recommend leaving it as is (or being the walk to
upstream pushes). Wrt upstream we make some assumptions in the impl here that
URLs use the HTML5 history methods and not hash based so its very much not ready
for inclusion.

https://codereview.appspot.com/7372044/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7372044/diff/1/app/app.js#newcode377
app/app.js:377: delete options.overrideAllNamespaces;
This is a very reasonable addition. I guessed we might need something like this
in the future, the future is now :)

https://codereview.appspot.com/7372044/diff/1/app/assets/javascripts/routing.js
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7372044/diff/1/app/assets/javascripts/routing....
app/assets/javascripts/routing.js:175: }
As clean as possible but no cleaner, presence may be as simple as indicating an
open panel state, are we sure we wouldn't wan't to include '/' if that is the
case? I wasn't sure. Your implication that its currently too hard to remove a
namespace from the path may be correct though. Something like override/removeNS
as you do in app.js is an option.

[gary.poster, 2013-02-21 17:19:55]

Please take a look.

[gary.poster, 2013-02-21 17:24:20]

On 2013/02/21 04:38:46, bcsaller wrote:
> Thanks for thinking more deeply about this code. Happy to talk about this more
> and I hope the inline comments are helpful.

Thank you, Ben!  They were.  I ended up asking Matt to take the _navigate code
changes in his logout branch, making the change to the routes seen mechanism,
and dumping the other non-comment changes I had made.  Specific replies to those
below.

> 
> https://codereview.appspot.com/7372044/diff/1/app/app.js
> File app/app.js (left):
> 
> https://codereview.appspot.com/7372044/diff/1/app/app.js#oldcode405
> app/app.js:405: self._dispatching = self._dispatched = true;
> This code is largely mapped from Y.Router, some of the base class code depends
> in part on these flags so I recommend leaving it as is (or being the walk to
> upstream pushes). Wrt upstream we make some assumptions in the impl here that
> URLs use the HTML5 history methods and not hash based so its very much not
ready
> for inclusion.

Gotcha.  I revert this, as mentioned.

> 
> https://codereview.appspot.com/7372044/diff/1/app/app.js
> File app/app.js (right):
> 
> https://codereview.appspot.com/7372044/diff/1/app/app.js#newcode377
> app/app.js:377: delete options.overrideAllNamespaces;
> This is a very reasonable addition. I guessed we might need something like
this
> in the future, the future is now :)

:-) Sent to Matt, as I said.

https://codereview.appspot.com/7372044/diff/1/app/assets/javascripts/routing.js
> File app/assets/javascripts/routing.js (right):
> 
>
https://codereview.appspot.com/7372044/diff/1/app/assets/javascripts/routing....
> app/assets/javascripts/routing.js:175: }
> As clean as possible but no cleaner, presence may be as simple as indicating
an
> open panel state, are we sure we wouldn't wan't to include '/' if that is the
> case? I wasn't sure. Your implication that its currently too hard to remove a
> namespace from the path may be correct though. Something like
override/removeNS
> as you do in app.js is an option.

I think this might be an important use case, but your point is well taken and
the easiest thing to do is declare YAGNI-for-now.  I removed this.

Thanks again

Gary

[bcsaller, 2013-02-21 17:26:41]

LGTM, thanks!

https://codereview.appspot.com/7372044/diff/6001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7372044/diff/6001/app/app.js#newcode422
app/app.js:422: this._routeSeen = {};
This is a nice cleanup. It wasn't possible before I figured out how to
centralize the code in _dispatch (parts of it used to live in dispatch, _save
and here) and they interacted with routeGeneration differently. This is cleaner
now, thanks.

https://codereview.appspot.com/7372044/diff/6001/app/app.js#newcode995
app/app.js:995: * without namespaces.
Y.Route.match is used to match routes via a regex. The plan is to extend this to
pass an optional namespace and use a namespace attr on the route to restrict
matches only within a given namespace. I still think '*' for middleware is
important to support and that happens here.

[bac, 2013-02-21 17:36:41]

LGTM.  Again, thanks for taking the time to carefully comment the code once you
figured it out.

https://codereview.appspot.com/7372044/diff/6001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7372044/diff/6001/app/app.js#newcode236
app/app.js:236: // These two attributes are used by the namespaced URL tracker.
Would you standardize on URL vs url, at least in this file?  I'd prefer the
former but would be happy just for consistency.

https://codereview.appspot.com/7372044/diff/6001/app/assets/javascripts/routi...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7372044/diff/6001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:201: // original value).
thanks for adding these comments.  they are really helpful.

[gary.poster, 2013-02-21 17:42:55]

Thank you both for the reviews!

https://codereview.appspot.com/7372044/diff/6001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7372044/diff/6001/app/app.js#newcode236
app/app.js:236: // These two attributes are used by the namespaced URL tracker.
On 2013/02/21 17:36:41, bac wrote:
> Would you standardize on URL vs url, at least in this file?  I'd prefer the
> former but would be happy just for consistency.

Done.

https://codereview.appspot.com/7372044/diff/6001/app/app.js#newcode995
app/app.js:995: * without namespaces.
On 2013/02/21 17:26:41, bcsaller wrote:
> Y.Route.match is used to match routes via a regex. The plan is to extend this
to
> pass an optional namespace and use a namespace attr on the route to restrict
> matches only within a given namespace. I still think '*' for middleware is
> important to support and that happens here.

As we said on IRC, this sounds like a good plan to me.  Thanks!

[gary.poster, 2013-02-21 17:46:49]

*** Submitted:

Explanation and cleanups for tardis URL code

This branch is largely comments, plus a single cleanup of the "routes seen per
URL" mechanism that I think is easier to understand with no downsides.

R=bcsaller, bac
CC=
https://codereview.appspot.com/7372044