-
-
Notifications
You must be signed in to change notification settings - Fork 16.7k
Call out that teardown functions must never raise #1451
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
Call out that teardown functions must never raise #1451
Conversation
Also add a more detailed explanation of how the request context and app context interact.
|
I should probably link my StackOverflow question that prompted this PR: http://stackoverflow.com/questions/23301968/invalid-transaction-persisting-across-requests |
|
+1 for merging this as it'd cut down on bug reports hopefully. Probably a good idea to merge into |
|
@jeffwidman I invited you as member of pallets, feel free to do this one yourself! |
|
Thanks. I'll cherrypick this into @untitaker are there any guidelines for what gets merged into maintenance branches? There have been many docs changes over the past few years that could have been merged into the |
|
@jeffwidman Generally we only merge doc improvements of any kind into master, since the maintenance branches are really only for actual bugfixes in the code. |
|
@untitaker Put it this way: how can we make doc improvements that folks can see when they go to flask.pocoo.org and click "Docs"? |
|
@garaden When the next release (0.11, 1.0, 1.1, 1.2) is out, these changes will be published there. The reason is that we don't want to represent features in the docs that are only in master, not any stable version. |
|
@untitaker Right, that makes total sense. It'd be super confusing if stable docs referenced features that aren't in the stable version. That said, I still think this PR belongs in the stable docs. The app context teardown functionality is already present, documented, and widely used, in the current stable release (0.10.x). This PR fixes an omission in that existing documentation: I think there are plenty of Flask 0.10.x users who don't know how much trouble can be caused by allowing a teardown function to raise, or what to do to prevent it. In that sense, I think it is a bugfix, even if it's not in the actual code and doesn't result in a new stable version. |
|
I don't consider documentation improvements of any kind to be bugfixes that need to be released as a bugfix release. Especially considering this PR does more than change a simple incorrect statement. |
|
@garaden Another aspect to consider is: Links to stable documentation are made to be permanent (which is why when browsing the documentation, you always have the version number in the URL). If you quote something from the documentation, and link to a page of the 0.10 docs, and later the entire structure of the page changes, how would you feel? |
|
@untitaker It's definitely a grey area. If the documentation was completely wrong (like saying a parameter defaults to Ideally, I'd like to see this PR make it to the docs on flask.pocoo.org, without needing a tag or PyPI release. I was hoping that merging it to the maintenance branch, without tagging the merge commit, would accomplish this. Then again...I have to admit that's a weird thing to do. "All maintenance branch changes result in a new tag & release" sounds like a reasonable rule, and this PR breaks that rule. Of course, this is your repo, not mine, so in the end the decision is yours :D I'm also glad to see an organization taking some of the weight of Flask & friends off of mitsuhiko. Major thanks for taking on that responsibility! That also raises a question: off the record, given the repo changing hands, can we expect new Flask releases in the near future, say a year or so? I'd feel better about merging this only to TL;DR: Are there any next actions for me here? I can close this PR and reopen against |
That would be much appreciated. |
|
Closing per @jeffwidman . |
Also add a more detailed explanation of how the request context and app
context interact.