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

Add route prefix macro and specs #1121

Merged
merged 10 commits into from
May 2, 2020

Conversation

rnice01
Copy link
Contributor

@rnice01 rnice01 commented Apr 26, 2020

Fixes #1087

Purpose

This PR is for issue #1087 for adding a route_prefix macro for Lucky::Actions. This will prefix all the routes added by the HTTP method and match macros.

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

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.

Super stoked for this! A few of my apps already need something like this.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

This looks really good! Just to make sure we don't have any regressions could you add a couple extra specs?

  • Layered inheritance. So maybe a abstract class TestApiPrefixAction < TestAction and then another action ChildApiPrefixAction < TestApiPrefixAction. It could have just one route and the spec would check that it still correctly prefixed the route
  • Modules. It may be helpful/common for people to make a route prefix module that is added to multiple actions. Could you create a ApiPrefix module and then an action that includes it and see if the action gets the prefix correctly?

Something like:

module ApiPrefix
  macro included
    route_prefix "/api"
  end
end

Does that make sense?

@rnice01
Copy link
Contributor Author

rnice01 commented Apr 28, 2020

This looks really good! Just to make sure we don't have any regressions could you add a couple extra specs?

  • Layered inheritance. So maybe a abstract class TestApiPrefixAction < TestAction and then another action ChildApiPrefixAction < TestApiPrefixAction. It could have just one route and the spec would check that it still correctly prefixed the route
  • Modules. It may be helpful/common for people to make a route prefix module that is added to multiple actions. Could you create a ApiPrefix module and then an action that includes it and see if the action gets the prefix correctly?

Something like:

module ApiPrefix
  macro included
    route_prefix "/api"
  end
end

Does that make sense?

@paulcsmith, for the Layered Inheritance spec, would the abstract class use the route_prefix macro and test the child correctly prefixes it's own routes from the parent's defined prefix?

`
abstract class ApiTestPrefixAction < TestAction
route_prefix "/api"
end

class ChildApiTestPrefixAction < ApiTestPrefixAction
get "/test" do
#make sure this gets prefixed
end
end
`

@paulcsmith
Copy link
Member

@rnice01 Yeah that's exactly what I had in mind! 👍

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Thanks for doing this :)

@paulcsmith paulcsmith merged commit b59d48d into luckyframework:master May 2, 2020
@jwoertink
Copy link
Member

oh man! Stoked for this ❤️

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.

Add route_prefix macro for prefixing/namespacing routes
3 participants