-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Remove get_redirect_route() in main.py, if possible #1726
Comments
@raman-balyan -- not sure if @ishankhare07 is still working on this. Would you like to have a go? |
@seanlip This is my first open source project. A bit nervous but ready to take it, if @ishankhare07 is not working.... |
OK, go for it! :) If you need help, feel free to ask on the gitter chat. |
Sure :) |
Hi @raman-balyan -- any luck? |
(De-assigning @raman-balyan due to lack of response.) |
Hi @yjli216 did you fix this? If so then please could you merge the current develop branch into your branch, and then create a PR so someone can review your code. Thanks! |
Hey! @yjli216 -- Ping ? |
In main.py, in the urls list at the bottom, there's a get_redirect_route() function which is somewhat confusing. For one thing, the third argument to this function is not used at all. It would be nice to find a simpler way of doing this.
One obvious idea is to give the "name" argument in get_redirect_route() a default value, but we'd need to check if it's OK for different routes to have the same name. Or maybe we can generate the name from the regex route somehow. Or maybe we can just get rid of get_redirect_route() altogether, and find another, cleaner way to achieve the same effect.
The main thing to check is that all tests still pass (including e2e ones) and that a request to /foo/ gets handled the same way as a request to /foo.
EDIT: To be clear, the idea behind this issue is to get rid of the third argument to get_redirect_route, which we don't really use and is therefore superfluous. One possibility might be to get rid of the whole get_redirect_route function itself, but if doing so means that stuff needs to be duplicated on all the lines, then let's not do that, since it violates the DRY principle.
/cc @charisseysabel
The text was updated successfully, but these errors were encountered: