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.

Sign up for GitHub

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

Jump to bottom

[Redirect] Allows for Class and Symbol Controller name #363

Merged
merged 9 commits into from Nov 8, 2017
Merged

[Redirect] Allows for Class and Symbol Controller name #363

merged 9 commits into from Nov 8, 2017

Conversation

eliasjpr
Copy link
Contributor

@eliasjpr eliasjpr commented Nov 5, 2017

Issue: #353

As a developer I would like to pass and full controller name to a
redirect as a class or symbol and allow for the redirection to happen

Example

redirect_to controller: :HomeController, action: :index

  • Would be nice to accept HelloController::Index or
    Admin::UserController:Index

@eliasjpr eliasjpr requested review from a team November 5, 2017 14:25
@eliasjpr eliasjpr mentioned this pull request Nov 5, 2017
Closed
@eliasjpr eliasjpr changed the title [REDIRECT] Allows for Class and Symbol Full Controller name [WIP][REDIRECT] Allows for Class and Symbol Full Controller name Nov 5, 2017
faustinoaq
faustinoaq approved these changes Nov 5, 2017
View reviewed changes
Copy link
Contributor

@faustinoaq faustinoaq left a comment •
edited

Choose a reason for hiding this comment

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

Do you think we should use this feature on generators by default ?

So, Instead of redirect_to "/posts", we would see redirect_to PostController, :index

:)

@eliasjpr
Copy link
Contributor Author

eliasjpr commented Nov 5, 2017

That would be a great idea. We should also have the ‘link_to’ jasper helpers user class name

@elorest
Copy link
Member

elorest commented Nov 5, 2017

We need to create url_for and have link_to and redirect_to translate links with that like in rails.

@eliasjpr eliasjpr force-pushed the ep/redirect-error-nil-assertion branch 3 times, most recently from a87cb04 to 5d96866 Compare November 7, 2017 00:38
@eliasjpr eliasjpr force-pushed the ep/redirect-error-nil-assertion branch from 5d96866 to c3857c4 Compare November 7, 2017 00:39
@eliasjpr eliasjpr changed the title [WIP][REDIRECT] Allows for Class and Symbol Full Controller name [REDIRECT] Allows for Class and Symbol Full Controller name Nov 7, 2017
@eliasjpr eliasjpr changed the title [REDIRECT] Allows for Class and Symbol Full Controller name [Redirect] Allows for Class and Symbol Full Controller name Nov 7, 2017
elorest
elorest reviewed Nov 7, 2017
View reviewed changes
Copy link
Member

@elorest elorest left a comment

Choose a reason for hiding this comment

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

Would this work with a url in which :id or other :params are in the middle of the urls such as posts/1/edit?

@eliasjpr
Copy link
Contributor Author

eliasjpr commented Nov 7, 2017 •
edited

A couple of changes introduced in the last 2 commits. I have decided to simplified the redirect api.

There is only 3 possible ways to redirect now:

  • Redirecting to full URL will be redirect_to "http://google.com", params. String based redirect are assumed that the user is passing the full path or url to redirect.
  • Redirecting controller action redirect_to PostController, :index, redirect_to :post, :index
  • Redirecting withing same controller redirect_to :index whichever controller this is called from it will be implicitly added.

Why?

  • This keeps things simple, coherent and short.
  • Supporting redirect_to :PostController, :index or redirect_to "PostController", :index makes things messy and confusing
  • People can pass controller name as symbols without the "controller" part, keeps it short.

@eliasjpr eliasjpr force-pushed the ep/redirect-error-nil-assertion branch 2 times, most recently from 567e6e0 to ee61ce1 Compare November 7, 2017 22:14
eliasjpr
eliasjpr commented Nov 7, 2017
View reviewed changes
src/amber/router/route.cr Outdated
@@ -49,7 +49,11 @@ module Amber
{result, params}
end

def match?(controller, action)
def match?(controller : Class, action : Symbol)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is separated into 2 method so we don't have to perform checks or substitutions for /Controller$/

eliasjpr and others added 6 commits November 7, 2017 18:09
@eliasjpr
[REDIRECT] Allows for Class and Symbol Full Controller name
00f5284 
Issue: #353

> As a developer I would like to pass and full controller name to a
redirect as a class or symbol and allow for the redirection to happen

Example

`redirect_to controller: :HomeController, action: :index`

- Would be nice to accept HelloController::Index or
Admin::UserController:Index
@eliasjpr
fixup! [REDIRECT] Allows for Class and Symbol Full Controller name
13515de
@eliasjpr
Adds Hash Table Lookup for Routes
9f4a679 
Looking up routes base on controller and action in a radix tree causes On2
many cases because of the radix tree.

To avoid that pain this adds hash table to perform lookups by conntroller
name and action while this might consume more memory it performs better and
makes the code cleaner and lookups based on controller#action are now more reliable

Signed-off-by: Elias Perez <eliasjpr@gmail.com>
@eliasjpr
Removes Redirect Exception
0c94e6b 
Since it is now using hash table lookup for redirects there is no need for
raising errors, if the controller action combination does not exists it throws
KeyError exception.

Signed-off-by: Elias Perez <eliasjpr@gmail.com>
@eliasjpr
Cleanup and fixes specs
77373cf 
Signed-off-by: Elias Perez <eliasjpr@gmail.com>
@eliasjpr
Cleanup
451c9c5
@eliasjpr eliasjpr force-pushed the ep/redirect-error-nil-assertion branch from 6cb2fdc to 451c9c5 Compare November 7, 2017 23:09
@eliasjpr eliasjpr changed the title [Redirect] Allows for Class and Symbol Full Controller name [Redirect] Allows for Class and Symbol Controller name Nov 7, 2017
@eliasjpr eliasjpr merged commit 83a4005 into master Nov 8, 2017
@eliasjpr eliasjpr deleted the ep/redirect-error-nil-assertion branch November 8, 2017 18:57
This was referenced Nov 12, 2017
Closed
Closed
elorest pushed a commit that referenced this pull request Nov 17, 2017
@eliasjpr
[Redirect] Allows for Class and Symbol Controller name (#363)
656c3f6 
* [REDIRECT] Allows for Class and Symbol Full Controller name

Issue: #353

> As a developer I would like to pass and full controller name to a
redirect as a class or symbol and allow for the redirection to happen

Example

`redirect_to controller: :HomeController, action: :index`

- Would be nice to accept HelloController::Index or
Admin::UserController:Index

* fixup! [REDIRECT] Allows for Class and Symbol Full Controller name

* Adds Hash Table Lookup for Routes


Looking up routes base on controller and action in a radix tree causes On2
many cases because of the radix tree.

To avoid that pain this adds hash table to perform lookups by conntroller
name and action while this might consume more memory it performs better and
makes the code cleaner and lookups based on controller#action are now more reliable

Signed-off-by: Elias Perez <eliasjpr@gmail.com>

* Removes Redirect Exception

Since it is now using hash table lookup for redirects there is no need for
raising errors, if the controller action combination does not exists it throws
KeyError exception.

Signed-off-by: Elias Perez <eliasjpr@gmail.com>

* Cleanup and fixes specs

Signed-off-by: Elias Perez <eliasjpr@gmail.com>

* Cleanup
elorest pushed a commit that referenced this pull request Nov 17, 2017
@eliasjpr
[Redirect] Allows for Class and Symbol Controller name (#363)
3d4d52c 
* [REDIRECT] Allows for Class and Symbol Full Controller name

Issue: #353

> As a developer I would like to pass and full controller name to a
redirect as a class or symbol and allow for the redirection to happen

Example

`redirect_to controller: :HomeController, action: :index`

- Would be nice to accept HelloController::Index or
Admin::UserController:Index

* fixup! [REDIRECT] Allows for Class and Symbol Full Controller name

* Adds Hash Table Lookup for Routes


Looking up routes base on controller and action in a radix tree causes On2
many cases because of the radix tree.

To avoid that pain this adds hash table to perform lookups by conntroller
name and action while this might consume more memory it performs better and
makes the code cleaner and lookups based on controller#action are now more reliable

Signed-off-by: Elias Perez <eliasjpr@gmail.com>

* Removes Redirect Exception

Since it is now using hash table lookup for redirects there is no need for
raising errors, if the controller action combination does not exists it throws
KeyError exception.

Signed-off-by: Elias Perez <eliasjpr@gmail.com>

* Cleanup and fixes specs

Signed-off-by: Elias Perez <eliasjpr@gmail.com>

* Cleanup

Former-commit-id: 656c3f6
@faustinoaq faustinoaq added this to Done in Framework 2018 May 5, 2018
@faustinoaq faustinoaq removed this from Done in Framework 2018 Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elorest elorest elorest left review comments

@faustinoaq faustinoaq faustinoaq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Amber Framework 2017
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@eliasjpr @elorest @faustinoaq