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

Infer route in action generators instead of using route macro #1378

Conversation

matthewmcgarvey
Copy link
Member

@matthewmcgarvey matthewmcgarvey commented Dec 31, 2020

Purpose

Fixes #1097

Description

Before we were utilizing the route macro in generated actions to infer the route at compile-time. This updates moves the route inferring into the generator.

$ lucky gen.action.browser Users::Index #=> route is GET /users
$ lucky gen.action.browser Users::Show #=> route is GET /users/:user_id
$ lucky gen.action.browser Users::Posts::Delete #=> route is DELETE /users/posts/:post_id
$ lucky gen.action.browser Users::GoOnline #=> fails because `GoOnline` is not a resourceful action

$ lucky gen.resource.browser User name:String #=> Creates all the routes with correct paths

Breaking Change:

  • Generation of actions will fail if not ending with a resourceful action (Index, Show, etc.)

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

@matthewmcgarvey matthewmcgarvey changed the title Infer route in action generators and remove route macro Infer route in action generators instead of using route macro Dec 31, 2020
@@ -0,0 +1,99 @@
class Lucky::RouteInferrer
Copy link
Member Author

Choose a reason for hiding this comment

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

@stephendolan
Copy link
Member

I'm not sure how much more complexity it would add, but is it possible to give users an escape hatch when requesting a non-restful route rather than throwing an error?

I was just setting up OAuth today, for example, and ran a bunch of generators like lucky gen.action.browser OAuth::Handlers::Callback that were really handy.

What if we just fallback to a get, substitute the :: for /, and call it good?

@matthewmcgarvey
Copy link
Member Author

@stephendolan That makes sense but I think that should be a separate issue. While you were able to generate the action, you definitely must have gone in and removed the route macro usage because it would have failed to compile otherwise with an error that looks like:

CleanShot 2020-12-31 at 15 36 39@2x

This PR moves the error up to when generating the action, instead.

@stephendolan
Copy link
Member

Yes, that's true! I mostly just use it to quickly scaffold up folders and file names for me.

@matthewmcgarvey
Copy link
Member Author

@stephendolan O wait, this issue is exactly what you want #1098

That will be done after this one is merged in

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Looks great! It'll be nice to get rid of this thing. I left some comments on the escapes in the strings. I'm good with it either way. So if you prefer as is, feel free to merge in.

@matthewmcgarvey matthewmcgarvey merged commit 34ce023 into luckyframework:master Dec 31, 2020
@matthewmcgarvey matthewmcgarvey deleted the matthewmcgarvey/issue1097 branch December 31, 2020 21:23
@bcardiff
Copy link
Contributor

FYI I am getting the following error when building lucky.gen.action.browser (using master of all lucky repo). Could it be a missing requirement that is unnoticed in the specs when building everything together?

$ lucky init.custom lucky101 --api
$ shards

... stripped ...

Error target lucky.gen.action.browser failed to compile:
   Using compiled compiler at /Users/bcardiff/Projects/crystal/master/.build/crystal
   In src/precompiled_tasks/gen/action/browser.cr:3:26
   
    3 | Gen::Action::Browser.new.print_help_or_call(ARGV)
                                 ^-----------------
   Error: instantiating 'Gen::Action::Browser#print_help_or_call(Array(String))'
   
   
   In tasks/gen/action/browser.cr:5:1
   
    5 | class Gen::Action::Browser < LuckyCli::Task
        ^
   Error: expanding macro
   
   
   There was a problem expanding macro 'inherited'
   
   Called macro defined in lib/lucky_cli/src/lucky_cli/task.cr:2:3
   
    2 | macro inherited
   
   Which expanded to:
   
    >  1 |     PARSER_OPTS = [] of Symbol
    >  2 |     @positional_arg_count : Int32 = 0
    >  3 |     property option_parser : OptionParser = OptionParser.new
    >  4 |     property output : IO = STDOUT
    >  5 | 
    >  6 |     
    >  7 |       LuckyCli::Runner.tasks << self.new
    >  8 |     
    >  9 | 
    > 10 |     def name
    > 11 |       "gen.action.browser"
    > 12 |     end
    > 13 | 
    > 14 |     def help_message
    > 15 |       <<-TEXT
    > 16 |       #{summary}
    > 17 | 
    > 18 |       Run this task with 'lucky #{name}'
    > 19 |       TEXT
    > 20 |     end
    > 21 | 
    > 22 |     @[Deprecated("Set #output instead of passing in `io`")]
    > 23 |     def print_help_or_call(args : Array(String), io : IO)
    > 24 |       @output = io
    > 25 |       print_help_or_call(args)
    > 26 |     end
    > 27 | 
    > 28 |     def print_help_or_call(args : Array(String))
    > 29 |       if wants_help_message?(args)
    > 30 |         output.puts help_message
    > 31 |       else
    > 32 |         {% for opt in @type.constant(:PARSER_OPTS) %}
    > 33 |         set_opt_for_{{ opt.id }}(args)
    > 34 |         {% end %}
    > 35 |         option_parser.parse(args)
    > 36 |         call
    > 37 |       end
    > 38 |     end
    > 39 | 
    > 40 |     private def wants_help_message?(args)
    > 41 |       args.any? { |arg| {"--help", "-h", "help"}.includes?(arg) }
    > 42 |     end
    > 43 |   
   Error: instantiating 'call()'
   
   
   In tasks/gen/action/browser.cr:21:5
   
    21 | render_action_template(io, inherit_from: "BrowserAction")
         ^---------------------
   Error: instantiating 'render_action_template(IO::FileDescriptor)'
   
   
   In tasks/gen/action/action_generator.cr:19:8
   
    19 | if valid?
            ^-----
   Error: instantiating 'valid?()'
   
   
   In tasks/gen/action/action_generator.cr:28:47
   
    28 | name_is_present && name_matches_format && route_generated_from_action_name
                                                   ^-------------------------------
   Error: instantiating 'route_generated_from_action_name()'
   
   
   In tasks/gen/action/action_generator.cr:42:5
   
    42 | route
         ^----
   Error: instantiating 'route()'
   
   
   In tasks/gen/action/action_generator.cr:52:16
   
    52 | @route ||= Lucky::RouteInferrer.new(action_class_name: action_name).generate_inferred_route
                    ^-------------------
   Error: undefined constant Lucky::RouteInferrer

@jwoertink
Copy link
Member

Well that's an interesting error 🤨

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.

Action generator changes to infer route
4 participants