Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output routes in :html format #8499

Merged
merged 1 commit into from Dec 13, 2012

Conversation

schneems
Copy link
Member

By formatting routes for different media (txt/html) we can apply optimizations based on the format. We can include meta-data in the HTML to allow a rich experience while rendering and viewing the routes. This PR shows route helpers as they are used with the _path extension, it also has a javascript toggle on the top to switch to _url. This way the developer can see the exact named route helper they can use instead of having to modify a base.

This is one example of an optimization that could be applied. Eventually we can link out to guides for the different columns to better explain what helper, HTTP Verb, Path, and Controller#action indicate. We could even add a route search box that could allow developers to input a given route and see all of the routes that match it. These are stand alone features and should be delivered separately.


By formatting routes for different media (txt/html) we can apply optimizations based on the format. We can include meta-data in the HTML to allow a rich experience while rendering and viewing the routes. This PR shows route helpers as they are used with the `_path` extension, it also has a javascript toggle on the top to switch to `_url`. This way the developer can see the exact named route helper they can use instead of having to modify a base. 

This is one example of an optimization that could be applied. Eventually we can link out to guides for the different columns to better explain what helper, HTTP Verb, Path, and Controller#action indicate. We could even add a route search box that could allow developers to input a given route and see all of the routes that match it. These are stand alone features and should be delivered separately.
@steveklabnik
Copy link
Member

👍

@sheerun
Copy link
Contributor

sheerun commented Dec 12, 2012

sextant on steroids 👍

@rafaelfranca
Copy link
Member

I like it.

@spastorino @jeremy thoughts?

@jeremy
Copy link
Member

jeremy commented Dec 13, 2012

Dig the idea and end result. The hard-coded format conditionals seem funky, though.

@carlosantoniodasilva
Copy link
Member

It could have separate formatters that are passed to the format method. So instead of calling

format(..., :html)

You could then inject the formatter:

format(... HTMLFormatter.new)

Or use the other way around:

HTMLFormatter.new(inspector.collect_all_routes(_routes.routes)).format.

@rafaelfranca
Copy link
Member

Yes, I'd propose this refactoring after we accept the change.

@steveklabnik
Copy link
Member

Well, my vote is :shipit: then.

@gaurish
Copy link
Contributor

gaurish commented Dec 13, 2012

good idea 👍

@carlosantoniodasilva
Copy link
Member

@rafaelfranca right, seems 👍 to me so.

steveklabnik added a commit that referenced this pull request Dec 13, 2012
@steveklabnik steveklabnik merged commit ae68fc3 into rails:master Dec 13, 2012
@steveklabnik
Copy link
Member

Okay then!

@steveklabnik
Copy link
Member

Oh! @schneems I forgot to bug you about a CHANGELOG. Let's do that with the refactoring, eh?

@coreyhaines
Copy link
Contributor

Just a thought on an improvement. One confusion is the implicit "look at the route helper name above" if there isn't anything in the column. Could we have this repeat each route helper name in the left column?

Pretty neat to see this, though.

@jacortinas
Copy link
Contributor

It would be incredible if the first occurrence of the route helper name was bold or black and every duplicate occurrence was a lighter shade of gray. /cc @coreyhaines

@dhh
Copy link
Member

dhh commented Dec 13, 2012

How and where is this output used? It feels like half a PR.

@steveklabnik
Copy link
Member

It is in the new 'routes in your browser' functionality.

That said, it is half of one, as I misunderstood that the refactoring was
before this got merged. I talked to @schneems and he's doing that soon, so
I didn't bother reverting.

@dhh
Copy link
Member

dhh commented Dec 14, 2012

I'm a huge fan of getting more of this stuff into the browser. That's what the Conductor project was about. I'm not so big a fan of having all this HTML generation shoved into the routing code itself, though. IMO this should be a separate engine that follows a regular mvc setup.

If there are people here who are interested in working on this, lets revive the conductor project and let's make routes display the first use case.

But let's remove this HTML generation from the routing code and treat it as a model.

On Dec 14, 2012, at 0:09, Steve Klabnik notifications@github.com wrote:

It is in the new 'routes in your browser' functionality.

That said, it is half of one, as I misunderstood that the refactoring was
before this got merged. I talked to @schneems and he's doing that soon, so
I didn't bother reverting.

Reply to this email directly or view it on GitHub.

@steveklabnik
Copy link
Member

I'm not so big a fan of having all this HTML generation shoved into the routing code itself, though.

@dhh I agree. This is what @carlosantoniodasilva suggested here: #8499 (comment)

@schneems is gonna grab this, and if he doesn't in a few days, then I am, since it's my fault for merging early.

IMO this should be a separate engine that follows a regular mvc setup.

Check out #6696. Routes are now shown in the browser upon routing errors. It's not quite an Engine, exactly...

@dhh
Copy link
Member

dhh commented Dec 14, 2012

That's a nice use case for it. But if we're going to expose it elsewhere, I say we get started on that Conductor approach.

In the mean time it seems like we should revert this as this is never going to be the implementation of it?

@schneems
Copy link
Member Author

@dhh the original functionality was put in a Gem https://rubygems.org/gems/sextant that has had over thirty-three thousand downloads in the last few months, it was added into Rails 4 after some on the core team reached out to me. I wrote a blog post on the ideals behind this and my other work in Raise Hell: Better programming through Error Messages.

I teach Rails at the University of Texas and this functionality came straight from user research with my students, as well as requests from seasoned Rails devs. The Engine is very limited in capacity, i'll get to that but first a quick recap on reasoning behind adding this functionality:

  1. Moving the routing into the Rails app, eliminates the need to boot the app to spit out the routes (such as when running rake routes). For some larger apps i've worked on this cuts down the time to see the routes from 30 seconds + to nearly instantaneously. This insanely reduced time allows for quick iteration, and less hairpulling. The Engine can give us this, but that is about it.
  2. When you get a routing error, it's because you either mis-typed something or you misinterpreted something in your routes. I can't help with the first, but we can show you the result of your routes on the error page. This info is presented just-in-time and gives you everything you need to fix your mistake. An engine doesn't have the hooks to add information to one of the built in error pages without totally over-writing it.
  3. The intent of this yet to be completed PR as @steveklabnik mentioned, is to add semantics and additional info to the rendering of the information which is not possible while outputting to the console.
  4. While these three pieces are great for seasoned devs, where they really shines is for beginners. When someone is first learning Rails, Routing is one of the hardest things to grok, making this information harder to find, and longer to get to only adds to the pain and confusion. Having it baked into Rails means it is available out of the box, with no configuration needed means developers of all skill levels have this information at their fingertips.

If you want to revert this PR, I'm all for it... it is still a WIP, I ask that you don't revert the other functionality without the pieces in place to provide an adequate replacement. I would be interested in helping out more with adding in some of those hooks if you're willing to give me some guidance.

steveklabnik added a commit to steveklabnik/rails that referenced this pull request Dec 14, 2012
…te-inspector"

This reverts commit ae68fc3, reversing
changes made to 0262a18.

See here: rails#8499 (comment)
@steveklabnik
Copy link
Member

Reverted in 8554537 as requested.

@schneems
Copy link
Member Author

Thanks @steveklabnik

@dhh
Copy link
Member

dhh commented Dec 14, 2012

@schneems It's a great feature. What we are discussing here is the implementation. Having the routing code know how to format HTML doesn't feel like its the right level of abstraction. Just like your model classes don't format HTML either.

I could see returning a json representation that can be transformed into HTML where that makes sense.

@schneems
Copy link
Member Author

@dhh I agree completely that the implementation is flawed, returning a collection and passing that to a partial or moving to some kind of presenter would even be a big step forwards. I'll iterate on the implementation a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants