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

Remove get_redirect_route() in main.py, if possible #1726

Closed
seanlip opened this issue Apr 13, 2016 · 9 comments
Closed

Remove get_redirect_route() in main.py, if possible #1726

seanlip opened this issue Apr 13, 2016 · 9 comments
Assignees
Labels

Comments

@seanlip
Copy link
Member

seanlip commented Apr 13, 2016

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

@seanlip
Copy link
Member Author

seanlip commented May 10, 2016

@raman-balyan -- not sure if @ishankhare07 is still working on this. Would you like to have a go?

@raman-balyan
Copy link

@seanlip This is my first open source project. A bit nervous but ready to take it, if @ishankhare07 is not working....

@seanlip seanlip assigned raman-balyan and unassigned ishankhare07 May 10, 2016
@seanlip
Copy link
Member Author

seanlip commented May 10, 2016

OK, go for it! :) If you need help, feel free to ask on the gitter chat.

@raman-balyan
Copy link

Sure :)

@seanlip
Copy link
Member Author

seanlip commented May 28, 2016

Hi @raman-balyan -- any luck?

@seanlip
Copy link
Member Author

seanlip commented Jun 3, 2016

(De-assigning @raman-balyan due to lack of response.)

yjli216 added a commit to yjli216/oppia that referenced this issue Jun 23, 2016
@jacobdavis11
Copy link
Member

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!

@souravbadami
Copy link
Contributor

Hey! @yjli216 -- Ping ?

@seanlip seanlip assigned Andimeo and unassigned yjli216 Sep 16, 2016
@seanlip
Copy link
Member Author

seanlip commented Sep 16, 2016

(Deassigning @yjli216 due to lack of response.)

@Andimeo, want to take this on? Note prior work in #1726 (though this doesn't take the right approach).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants