Add websocket routes to 'amber routes' output (#295) #492

docelic · 2018-01-09T00:37:22Z

Show websocket routes in 'amber routes'
Avoid string "[Time] Class |" that used to be printed in the first line
of output
Fix general issue where in case of Con::Troller, only "Con" would be printed
in Controller column

- Show websocket routes in 'amber routes' - Avoid string "<Time> Class |" that used to be printed in first line of output - Fix issue where in case of Con::Troller, only "Con" would be printed in Controller column

marksiemers

Looks like a good approach.

I'll look into writing some specs. See inline comments.

marksiemers · 2018-01-09T00:40:45Z

src/amber/cli/commands/routes.cr

@@ -120,8 +128,7 @@ module Amber::CLI
            row.add_column route[l].to_s
          end
        end
-        puts table
-        exit


Any reason to remove exit?

Yes, there is no reason for the function named "print" to also exit automatically. I've moved "exit" to the run() method, where it properly exits after printing the table.

marksiemers · 2018-01-09T00:41:26Z

src/amber/cli/commands/routes.cr

@@ -5,9 +5,10 @@ require "../helpers/sentry"
 module Amber::CLI
  class MainCommand < ::Cli::Supercommand
    class Routes < Command
-      RESOURCE_ROUTE_REGEX = /(\w+)\s+\"([^\"]+)\",\s*(\w+)(?:,\s*(\w+)\:\s*\[([^\]]+)\])?/
-      VERB_ROUTE_REGEX     = /(\w+)\s+\"([^\"]+)\",\s*(\w+),\s*:(\w+)/
-      PIPE_SCOPE_REGEX     = /routes\s+\:(\w+)(?:,\s+\"([^\"]+)\")?/


Did any of these actually change? Or is this a whitespace diff?

They changed. Two changes are made:

A ":" is added inside the regex so that controllers containing ":" in the name would work properly

And () were added around 'routes' and 'websockets' words even though not strictly necessary technically. (I've added them for consistency with other regexes and future expansion to cover multiple words)

marksiemers · 2018-01-09T00:43:23Z

src/amber/cli/commands/routes.cr

-          @current_pipe = route_match[1]?
-          @current_scope = route_match[2]?
+          @current_pipe = route_match[2]?
+          @current_scope = route_match[3]?


Now I'm guessing those Regexs did change. Is there an extra capture group now?

Changed due to your #1 question above (my answer #2 there), so that's why the number increased.

marksiemers · 2018-01-09T00:46:18Z

src/amber/cli/commands/routes.cr

+            verb: route_match[1]?, controller: route_match[3]?,
+            action: "", pipeline: current_pipe,
+            scope: current_scope, uri_pattern: route_match[2]?
+          )


It looks like there is only one difference here:

build_route( verb: route_match[1]?, controller: route_match[3]?, action: "", pipeline: current_pipe, scope: current_scope, uri_pattern: route_match[2]? )

The action is an empty string when it is a websocket. Can this be DRYed up?

I was thinking this would be here for future more detailed separation/improvements to HTTP and websocket code paths which wouldn't be shared between both. But sure, can simplify for now, and the () around regexes that I've added at the top make this trivial. Will add a commit soon.

Actually, not simple to DRY. If you find a way, please improve upon my patch. Thanks.

marksiemers · 2018-01-09T03:00:59Z

@docelic - I added some tests in #494
I didn't have write access to your repo (I don't think) so I included your commit in another branch.

docelic · 2018-01-09T03:46:51Z

Great, I will close this PR and assume you'll handle #494. Thanks!

Add websocket routes to 'amber routes' output (#295)

a129be3

- Show websocket routes in 'amber routes' - Avoid string "<Time> Class |" that used to be printed in first line of output - Fix issue where in case of Con::Troller, only "Con" would be printed in Controller column

marksiemers reviewed Jan 9, 2018

View reviewed changes

docelic closed this Jan 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add websocket routes to 'amber routes' output (#295) #492

Add websocket routes to 'amber routes' output (#295) #492

docelic commented Jan 9, 2018 •

edited

Loading

marksiemers left a comment

marksiemers Jan 9, 2018

docelic Jan 9, 2018

marksiemers Jan 9, 2018

docelic Jan 9, 2018

marksiemers Jan 9, 2018

docelic Jan 9, 2018

marksiemers Jan 9, 2018

docelic Jan 9, 2018

docelic Jan 9, 2018 •

edited

Loading

marksiemers commented Jan 9, 2018

docelic commented Jan 9, 2018

Add websocket routes to 'amber routes' output (#295) #492

Add websocket routes to 'amber routes' output (#295) #492

Conversation

docelic commented Jan 9, 2018 • edited Loading

marksiemers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

docelic Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

marksiemers commented Jan 9, 2018

docelic commented Jan 9, 2018

docelic commented Jan 9, 2018 •

edited

Loading

docelic Jan 9, 2018 •

edited

Loading