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 syntatic sugar for route registration #3907

Merged
merged 1 commit into from Mar 8, 2021
Merged

Conversation

pgjones
Copy link
Member

@pgjones pgjones commented Feb 14, 2021

I know we briefly discussed this, but I'd like to make the case with a concrete implementation. My argument is that this is actually quite a nice API and requires little implementation (especially now with the nice Scaffold). There is also precedent, in my view, that the test client also has this API. (Also, everyone copies Flask so lets copy them :p).

Commit message

This takes a popular API whereby instead of passing the HTTP method as
an argument to route it is instead used as the method name i.e.

@app.route("/", methods=["POST"])

is now writeable as,

@app.post("/")

This is simply syntatic sugar, it doesn't do anything else, but makes
it slightly easier for users.

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@ThiefMaster
Copy link
Member

ThiefMaster commented Feb 14, 2021

What I dislike with this approach is that - at least when you aren't writing a RESTful API - there is a very good chance that you need endpoints handling both GET and POST (render form if not submitted or validation failed, otherwise do stuff). In fact, in a classic webapp with lots of forms it's probably the most common thing after GET endpoints...

However, this case is not covered by the shortcuts and I can't think of any decent name for a method that does cover it... And I think beginners may think mixed-method routes are discouraged because of this, making them do weird stuff with the session for the basic web form usecase described above...

@pgjones
Copy link
Member Author

pgjones commented Feb 14, 2021

I believe the test failures are unrelated.

I think the existing route method covers the GET & POST case you describe really well. There is (and will still be) a large corpus of tutorials and documentation for beginners that uses route, so I don't think we need worry about them going any further astray. As a counter I think it will make it easier for beginners writing RESTful APIs with this syntax.

This takes a popular API whereby instead of passing the HTTP method as
an argument to route it is instead used as the method name i.e.

    @app.route("/", methods=["POST"])

is now writeable as,

    @app.post("/")

This is simply syntatic sugar, it doesn't do anything else, but makes
it slightly easier for users.

I've included all the methods that are relevant and aren't auto
generated i.e. not connect, head, options, and trace.
@davidism
Copy link
Member

davidism commented Mar 8, 2021

Refactored to avoid repeating the error message in each method. Changed to a TypeError since that's what's used for issues with arguments. Added tests for all methods and for the error raised.

@davidism davidism merged commit 5fea7ca into pallets:master Mar 8, 2021
@davidism davidism added this to the 2.0.0 milestone Mar 8, 2021
@greyli
Copy link
Member

greyli commented Mar 9, 2021

Glad to see this finally merged! After I proposed this idea and was declined in the first pallets meeting, I started to make the REST API extension I'm developing to become a framework so that I can inherit the Flask class and add these route shortcuts, now I may consider making it back to an extension...

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants