View your Routes without waiting on Rake #6461

Merged
merged 2 commits into from May 24, 2012

Conversation

Projects
None yet
8 participants
Member

schneems commented May 23, 2012

Bring the functionality of Sextant natively to Rails 4. Rather than having to initialize your Rails application ever time you run $ rake routes we can utilize the currently running server to generate routes in much less time. To view routes go to /rails/info/routes, the section in <pre> will match $ rake routes

cc/ @pixeltrix

Member

josevalim commented May 23, 2012

Instead of defining rails/info/:id, please generate each route, this will
allow us to get rid of the unnecessary case in the controller.

Sent from my iPhone

Member

josevalim commented May 23, 2012

And pls don't add ## headings to comments. Thanks!

Sent from my iPhone

Member

schneems commented May 23, 2012

@josevalim removed ## on comment and moved to separate methods in the InfoController to get rid of the case statement. Let me know if there is anything else.

Member

arunagw commented May 23, 2012

Also please squash these commits into smaller number. May be 1-2.

@pixeltrix pixeltrix and 1 other commented on an outdated diff May 23, 2012

railties/lib/rails/application/route_inspector.rb
@@ -59,6 +59,20 @@ def engine?
end
end
+ # This class is just used for displaying route information
+ # People should not use this class.
+ class RoutePresenter # :nodoc:
+ def self.display_routes(routes = all_routes)
+ inspector = Rails::Application::RouteInspector.new
+ inspector.format(routes, ENV['CONTROLLER']).join "\n"
+ end
+
+ def self.all_routes
+ Rails.application.reload_routes!
@pixeltrix

pixeltrix May 23, 2012

Owner

Do we need to reload routes? They're reloaded on every request in development mode so are unlikely to be stale.

@schneems

schneems May 23, 2012

Member

I just tested this out, can confirm that reload is not needed, it can also be removed from the $ rake routes task as well, I can save that for another PR, or add on another commit here if you like

@pixeltrix

pixeltrix May 23, 2012

Owner

Keep the rake routes change for another pull request - just in case there's reason it's there it'll be easier to revert.

BTW, thanks for your work this. 👏

@pixeltrix pixeltrix commented on an outdated diff May 23, 2012

railties/lib/rails/info_controller.rb
def properties
- if consider_all_requests_local? || request.local?
- render :inline => Rails::Info.to_html
- else
- render :text => '<p>For security purposes, this information is only available to local requests.</p>', :status => :forbidden
- end
+ @info = Rails::Info.to_html
+ render "templates/properties", :layout => 'layout'
@pixeltrix

pixeltrix May 23, 2012

Owner

I'd prefer a templates directory within rails and then use the standard layouts/application and info/:action - that way if we add anything other than InfoController it can use the same directory structure.

@pixeltrix pixeltrix commented on an outdated diff May 23, 2012

railties/test/rails_info_controller_test.rb
@@ -34,7 +35,7 @@ def setup
test "info controller allows requests when all requests are considered local" do
@request.stubs(:local? => false)
- @controller.stubs(:consider_all_requests_local? => true)
+ Rails.application.config.stubs(:consider_all_requests_local => true)
@pixeltrix

pixeltrix May 23, 2012

Owner

I'd prefer to keep the controller method so we can stub it directly in these tests.

Owner

jeremy commented May 23, 2012

Hot. Happy to see more like this available in dev.

Member

schneems commented May 23, 2012

Squashed everything into two commits, removed the reload code, updated the controller stubs in the tests, moved views under rails/templates, if we want to encourage more of this type of functionality, I would suggest making a full blown app/ dir in rails/ and putting the controller in rails/app/controllers etc.

@pixeltrix pixeltrix and 2 others commented on an outdated diff May 23, 2012

railties/lib/rails/application/route_inspector.rb
@@ -59,6 +59,19 @@ def engine?
end
end
+ # This class is just used for displaying route information
+ # People should not use this class.
+ class RoutePresenter # :nodoc:
+ def self.display_routes(routes = all_routes)
+ inspector = Rails::Application::RouteInspector.new
+ inspector.format(routes, ENV['CONTROLLER']).join "\n"
@pixeltrix

pixeltrix May 23, 2012

Owner

Not sure there's much point in passing ENV['CONTROLLER'] here - it's used in the rake task to focus on a particular controller, e.g: rake routes CONTROLLER=products. Perhaps we could pass a url parameter instead, e.g:

# can't  use :controller here obviously
get '/rails/info/routes(/:filter)' => 'rails/info#routes'

class Rails::InfoController < ActionController::Base
  def routes
    @inspector = Rails::Application::RouteInspector.new
    @info = @inspector.format(_routes.routes, params[:filter]).join("\n")
  end
end

That way you don't need the RoutePresenter class.

A couple more points:

  1. Do you need the explicit render calls - does the default render not select the right templates?
  2. Do we need the explicit layout 'application' - does the layout not get selected correctly?

Not really bothered whether the explicit calls are there or not - I'm just wondering why you put them there.

@josevalim

josevalim May 23, 2012

Member

The filter option and routes are unnecessary for the web page. I'd rather
hit Cmd+F in my browser.

Sent from my iPhone

@schneems

schneems May 24, 2012

Member
  1. In my testing we needed to explicitly call layout or none will be rendered, not sure why.
  2. Didn't try not explicitly calling render, when I take those lines out, it works fine...good catch.

I can kill the presenter, going with the cmd+f option. I would eventually like to pass it a url /users/5/whatever/3 and have it give me all the routes it matches since we can't Cmd+F for that, but for now i'm looking to get this out the door. I can experiment with filters/layouts in my free time on sextant, and just port over the most relevant/used features.

Unfortunately _routes is a different class than expected by the inspector, final version will now look like this:

  def routes
    inspector = Rails::Application::RouteInspector.new
    @info     = inspector.format(Rails.application.routes.routes).join("\n")
  end
Member

schneems commented May 24, 2012

updated code to not use render and removed the presenter

Owner

pixeltrix commented May 24, 2012

Interesting about the layout - I'll have to check what's going on there. The _routes.routes should work but don't bother changing it - I'll merge it in as is.

Owner

pixeltrix commented May 24, 2012

Sorry, forgot one thing - it needs a CHANGELOG entry.

Member

josevalim commented May 24, 2012

This is looking good! 👏

schneems added some commits May 22, 2012

@schneems schneems /rails/info/routes path shows routing information
Will show similar contents to the output of `$ rake routes` in the browser in development. This speeds the time required to generate routes, since the application is already initialized.
cb44e0f
@schneems schneems Rails::InfoController tests passing
This includes new tests for /rails/info/routes
c3e3102
Member

schneems commented May 24, 2012

Switched over to _routes.routes to save a few characters. Added a change log entry, thanks for all the help guys. Hope you don't mind @pixeltrix, added you to the changelog.

@pixeltrix pixeltrix added a commit that referenced this pull request May 24, 2012

@pixeltrix pixeltrix Merge pull request #6461 from schneems/schneems/sextant-routes
View your Routes without waiting on Rake
8186754

@pixeltrix pixeltrix merged commit 8186754 into rails:master May 24, 2012

I don't see where the RoutePresenter is defined. Am I missing something?

Member

schneems replied May 25, 2012

Nope, we refactored it out, we need to remove this test, good catch. Want a PR?

Owner

rafaelfranca replied May 25, 2012

please.

Member

schneems replied May 25, 2012

Submitted PR #6481, sorry about that.

Great job guys!

@vijaydev vijaydev pushed a commit that referenced this pull request Jun 2, 2012

@schneems schneems routes are viewable in browser (update guides)
From the Pull Request #6461
e636663
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment