Added helper method redirect_url_for #958

Closed
wants to merge 1 commit into
from

Conversation

9 participants

redirect_url_for(endpoint, *_values) == redirect(url_for(endpoint,
*_values)

Added helper method redirect_url_for
redirect_url_for(endpoint, **values) == redirect(url_for(endpoint,
**values)
Owner

ThiefMaster commented Jan 22, 2014

I don't really like the function name (or the )... And adding a new function to save one character? Meh, let's not bloat Flask with stuff like that.

You can always create this function locally in your application anyway!

Owner

untitaker commented Jan 22, 2014

Yeah, neither do i think this will get included in Flask.

There should be one-- and preferably only one --obvious way to do it.

I don't really like the function name

Agree.

You can always create this function locally in your application anyway

Sure. But at least for me, redirecting to an endpoint is what I do 9 out of 10 times. So I believe that there should be a built-in, easier way to do that.

IMO it would be better if the signature of redirect was:

redirect(endpoint, **values)

Redirecting to an arbitrary location could be:

redirect(_location="http://...)

or similar.

This would obviously break backward compatibility.

Owner

untitaker commented Jan 22, 2014

Your new suggestion would, as you said, break backward compatibility, for IMO no gain other than a changed color of the bikeshed.

I believe that if the most frequent use case for redirect is redirect(url_for()), then it should be the default.

Grepping through some flask apps' code bases this seems to be the case (see below).

To avoid most backward compatibility issues, redirect() could be implemented similar to Django. Django's redirect function checks if the passed argument is a view name (equivalent to endpoint name in Flask) or a URL and does the right thing (see https://docs.djangoproject.com/en/1.6/topics/http/shortcuts/#redirect).

Examples of redirect usage in a few Flask apps:

flask/examples

$ find -name '*.py' | xargs grep -h "redirect("
        return redirect(url_for('public_timeline'))
    return redirect(url_for('user_timeline', username=username))
    return redirect(url_for('user_timeline', username=username))
    return redirect(url_for('timeline'))
        return redirect(url_for('timeline'))
            return redirect(url_for('timeline'))
        return redirect(url_for('timeline'))
            return redirect(url_for('login'))
    return redirect(url_for('public_timeline'))
    return redirect(url_for('show_entries'))
            return redirect(url_for('show_entries'))
    return redirect(url_for('show_entries'))

Application from "The Flask mega-tutorial"

$ find -name '*.py' | xargs grep -h "redirect("
        return redirect(url_for('index'))
        return redirect(url_for('index'))
        return redirect(url_for('login'))
    return redirect(request.args.get('next') or url_for('index'))
    return redirect(url_for('index'))
        return redirect(url_for('index'))
        return redirect(url_for('edit'))
        return redirect(url_for('index'))
        return redirect(url_for('user', nickname = nickname))
        return redirect(url_for('user', nickname = nickname))
    return redirect(url_for('user', nickname = nickname))
        return redirect(url_for('index'))
        return redirect(url_for('user', nickname = nickname))
        return redirect(url_for('user', nickname = nickname))
    return redirect(url_for('user', nickname = nickname))
        return redirect(url_for('index'))
        return redirect(url_for('index'))
    return redirect(url_for('index'))
        return redirect(url_for('index'))
    return redirect(url_for('search_results', query = g.search_form.search.data))

bf3-aggregator:

$ find -name '*.py' | xargs grep -h "redirect("
            return redirect(url_for('login', next=request.url))
    return redirect(url_for('favorites', page=page, slug=g.user.slug))
def show_developers_redirect():
    return redirect(url_for('show_developers'))
        return redirect(oid.get_next_url())
    return redirect(oid.get_next_url())
        return redirect(url_for('index'))
    return redirect(oid.get_next_url())
        return redirect(request.base_url)
Owner

untitaker commented Jan 22, 2014

You argue that using url_for in redirect is a common use-case, but fail to explain why a single function call would be more practical or less verbose than two nested ones. IMO the current way of doing things is extremely elegant as both functions serve a very generic purpose, while their combination is still not tedious to write and easy to read.

Your suggestions seems to me like an unneccessary extension of the API exposed by Flask. Depending on which is your current one, they introduce either new, radically different behavior of a function when it gets passed different keyword arguments, or add a new function that does little more than nest two functions which could've been nested with the same amount of keystrokes by the user himself.

You mention Django's redirect function which checks if the passed argument is either a model object, if there is a view named like it, or if it "feels" like an URL. If the redirect function would become like that, i am not so sure if the behavior of that function is as well-defined and easily understandable as it is now. For example, if the view that was passed would actually take an argument called code, should redirect('my_view', code=302) pass that argument on to the view, or use it for the status code of the redirect? If you think that redirect should always "catch" code, how about redirect('activate_coupon', code='4956n36')? I mean, a non-numeric value can't possibly be a status code, right? Should redirect try to use it as such anyway, just to throw up, or try to convert it to an integer first, then, if that fails, pass the code on to the view? Even if you think that we should just do it like in Django, and catch code at all times, or rename code to _code, i think this is going to be a pitfall for people in any case, and also a major break in backwards compatibility. Whereas having two separate functions gives the user a clear idea which arguments are used for which operation.

You argue that using url_for in redirect is a common use-case, but fail to explain why a single function call would be more practical or less verbose than two nested ones.

That a single call is more practical and less verbose than two nested calls is rather self-explanatory.

Your arguments regarding code are valid, there will be corner cases that are impossible to handle correctly.

But I still believe it would be worthwhile to simplify the most common redirect/url_for use case.

Owner

untitaker commented Jan 22, 2014

No, i don't think it's self-explanatory, especially since both forms are expressions and require roughly the same amount of characters.

Seems unnecessary. Its better to be verbose than add yet another method that's not really needed (it doesn't solve any actual problems) IMO.

We're discussing two different issues here:

  1. the redirect_url_for function
  2. if redirect should be changed to be redirect(endpoint, **values) where endpoint could possibly be an endpoint name or a URL

I agree that redirect_url_for doesn't really add much.

I'll make another pull request for issue 2.

Contributor

nZac commented Jan 23, 2014

@codeape2 Would you say then that this PR could be closed?

Contributor

jaapz commented Jan 23, 2014

I am +1 for the second proposal (redirect accepting both view names and URL's). In my apps i have actually never used redirect to redirect anywhere but some view in my app. So i guess it's a commen enough use case to change the api for this.

Contributor

mattupstate commented Jan 24, 2014

redirect is just an import from werkzeug and it is ignorant to the URL map of the application. It would seem that you'd have to write a wrapper function which can be executed within an app context in order to achieve the desired API. FWIW, I don't see a need for this functionality myself. And perhaps explicit is better than implicit applies to this situation.

Owner

mitsuhiko commented Feb 8, 2014

Not going to land. It's not solving any issues and just making the API more complex.

@mitsuhiko mitsuhiko closed this Feb 8, 2014

Alternate suggestion:

Call the composite method redirect_to, like webapp2 does:

https://webapp-improved.appspot.com/api/webapp2.html#webapp2.redirect_to

This saves significantly more characters than the original proposal, so it's probably worth it in that regard.
It does not break compatibility with existing apps, while providing some value to new ones. While the name is not the most obvious and does indeed make the API a bit more complex, I could argue that it's the least surprising way of doing things for people coming from a webapp2 background (and while we're quoting the Zen, that's the approach Python took with regards to most system calls). Since webapp2 is the default framework for Google App Engine, and both webapp2 and GAE suck big time [citation needed], I for one am expecting a large surge of developers switching to flask in the near future 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment