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

Change message on the empty result of searching routes by rails routes with -c or -g #32153

Merged
merged 2 commits into from
Mar 13, 2018

Conversation

bogdanvlviv
Copy link
Contributor

@bogdanvlviv bogdanvlviv commented Mar 2, 2018

  • Improve docs of ActionDispatch::Routing

    • Add a mention about -g.
    • Improve info about --expanded option of rails routes.
  • Introduce ActionDispatch::Routing::ConsoleFormatter::Base

    • Create Base and inherit Sheet and Expanded in order to
      prevent code duplication.
      • Remove trailing "\n" for components of Expanded.
      • There is no need for Expanded#header to return @buffer so return nil instead.
    • Change no_routes message "No routes were found for this controller"
      since if use -g, it sounds incorrect.
      • Display No routes were found for this controller. if apply -c.
      • Display No routes were found for this grep pattern. if apply -g.

Related to #32130

@rails-bot
Copy link

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

# Target specific controllers by prefixing the command with <tt>-c</tt> option. Use
# <tt>--expanded</tt> to turn on the expanded table formatting mode.
#
# Target specific controller by prefixing the command with <tt>-c</tt> option,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's strike the 2 option occurences. The sentences sound better without them.

It should be: Target a specific.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by prefixing the command with -c option

That doesn't sound right; options are passed after the command. I'd ✂️ "by prefixing the command" here.

#
# Target specific controller by prefixing the command with <tt>-c</tt> option,
# or grep routes using <tt>-g</tt> option. Use <tt>--expanded</tt> to turn on
# the expanded table formatting mode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the expanded table formatting mode doesn't really explain anything to users. How about:

# Useful in conjunction with <tt>--expanded</tt> which displays routes vertically.

MESSAGE
else
"No routes were found for this controller\n"
"No routes were found."
end
@buffer << "For more information about routes, see the Rails guide: http://guides.rubyonrails.org/routing.html."
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we should figure out a way to share the 2 no_routes methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'll create Base class and inherit from it.

MESSAGE
else
"No routes were found for this controller\n"
"No routes were found."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should tune this to No routes found matching conditions. to indicate to the user that their -g or -c was applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaspth What do you think about displaying a message depending on which option was passed -c or -g?

wip: draft
diff --git a/actionpack/lib/action_dispatch/routing/inspector.rb b/actionpack/lib/action_dispatch/routing/inspector.rb
index 73f0531..36128b3 100644
--- a/actionpack/lib/action_dispatch/routing/inspector.rb
+++ b/actionpack/lib/action_dispatch/routing/inspector.rb
@@ -64,7 +64,7 @@ def format(formatter, filter = nil)
         routes_to_display = filter_routes(normalize_filter(filter))
         routes = collect_routes(routes_to_display)
         if routes.none?
-          formatter.no_routes(collect_routes(@routes))
+          formatter.no_routes(collect_routes(@routes), filter)
           return formatter.result
         end
 
@@ -82,9 +82,9 @@ def format(formatter, filter = nil)
       private
 
         def normalize_filter(filter)
-          if filter.is_a?(Hash) && filter[:controller]
+          if filter[:controller]
             { controller: /#{filter[:controller].downcase.sub(/_?controller\z/, '').sub('::', '/')}/ }
-          elsif filter
+          elsif filter[:grep_pattern]
             { controller: /#{filter}/, action: /#{filter}/, verb: /#{filter}/, name: /#{filter}/, path: /#{filter}/ }
           end
         end
@@ -147,7 +147,7 @@ def header(routes)
           @buffer << draw_header(routes)
         end
 
-        def no_routes(routes)
+        def no_routes(routes, filter)
           @buffer <<
           if routes.none?
             <<~MESSAGE
@@ -205,7 +205,7 @@ def section(routes)
         def header(routes)
         end
 
-        def no_routes(routes)
+        def no_routes(routes, filter)
           @buffer <<
           if routes.none?
             <<~MESSAGE
@@ -213,8 +213,10 @@ def no_routes(routes)
 
             Please add some routes in config/routes.rb.
             MESSAGE
-          else
-            "No routes were found."
+          elsif filter.has_key?(:controller)
+            "No routes were found for this controller."
+          elsif filter.has_key?(:grep_pattern)
+            "No routes were found for this pattern."
           end
           @buffer << "For more information about routes, see the Rails guide: http://guides.rubyonrails.org/routing.html."
         end
diff --git a/railties/lib/rails/commands/routes/routes_command.rb b/railties/lib/rails/commands/routes/routes_command.rb
index c4f3717..859ab9c 100644
--- a/railties/lib/rails/commands/routes/routes_command.rb
+++ b/railties/lib/rails/commands/routes/routes_command.rb
@@ -39,7 +39,7 @@ def routes_filter
           if options.has_key?("controller")
             { controller: options["controller"] }
           elsif options.has_key?("grep_pattern")
-            options["grep_pattern"]
+            { grep_pattern: options["grep_pattern"] }
           else
             nil
           end

@bogdanvlviv bogdanvlviv changed the title Change info about the empty result on rails routes Change message the empty result of searching routes by rails routes with -c or -g Mar 5, 2018
@bogdanvlviv bogdanvlviv changed the title Change message the empty result of searching routes by rails routes with -c or -g Change message on the empty result of searching routes by rails routes with -c or -g Mar 5, 2018
@bogdanvlviv
Copy link
Contributor Author

Thanks for the review! I just added suggested changes. What do you think about #32153 (comment)?

@bogdanvlviv bogdanvlviv force-pushed the rails-routes-32130 branch 2 times, most recently from 2a54051 to 927a968 Compare March 11, 2018 19:59
@kaspth
Copy link
Contributor

kaspth commented Mar 11, 2018

@bogdanvlviv 👍, if we can use symbolize_keys.slice(:controller, :grep_pattern) that might be better too (can't recall if we have Active Support access there).

@bogdanvlviv
Copy link
Contributor Author

@kaspth I implemented this in 2eff35809aa19cf4dc586b55a2fa59ce33d8d2e3. Let me know if you have concerns about the content of messages.

@kaspth
Copy link
Contributor

kaspth commented Mar 12, 2018

All right, lets squash the commits then we're good 👍

- Add a mention about `-g`.
- Improve info about `--expanded` option of `rails routes`.
- Create `Base` and inherit `Sheet` and `Expanded` in order to
- prevent code duplication.
  - Remove trailing "\n" for components of `Expanded`.
  - There is no need for `Expanded#header` to return `@buffer` so return `nil` instead.

- Change `no_routes` message "No routes were found for this controller"
  since if use `-g`, it sounds incorrect.
  - Display `No routes were found for this controller.` if apply `-c`.
  - Display `No routes were found for this grep pattern.` if apply `-g`.

Related to rails#32130
@bogdanvlviv
Copy link
Contributor Author

bogdanvlviv commented Mar 13, 2018

Squashed commits related to change of messages on an empty result of bin/rails routes into one commit.
Fixing docs leaves in separate commit(e78b1e5) since it isn't related to 304906f.

@kaspth kaspth merged commit bebce8c into rails:master Mar 13, 2018
@kaspth
Copy link
Contributor

kaspth commented Mar 13, 2018

Solid 👍

@bogdanvlviv bogdanvlviv deleted the rails-routes-32130 branch March 13, 2018 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants