GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||
GeezerVison is showing github.com — gh-101693: In sqlite3, deprecate using named placeholders with parameters supplied as a sequence by erlend-aasland · Pull Request #101698 · python/cpython Conversation
|
|
Alex, would you mind taking a look at the docs/NEWS/What's New changes? I feel the sentences grew to be a little bit too complex. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Also, the text in the deprecation warning is perhaps a little bit too long 😶 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
executemany only works with data-modifying queries
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a case where a warning would be better than an exception, but so be it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thanks for your review, Serhiy; highly appreciated. @AlexWaygood, do you still want to take a look? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
A few more minor points
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| and there is no open transaction, | ||
| a transaction is implicitly opened before executing *sql*. | ||
|
|
||
| .. versionchanged:: 3.12 |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| .. versionchanged:: 3.12 | |
| .. deprecated-removed:: 3.12 3.14 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about that? The API itself is not deprecated, we're just changing it. I'm not sure what's the recommended practice here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know what you mean. I feel like ideally this notice would be phrased along the lines of "X behaviour/practice is now deprecated" rather than "a DeprecationWarning is now emitted". That would be more to-the-point, and it would also work more naturally with this directive (X behaviour/practice is deprecated in 3.12, and will be removed entirely in 3.14).
But I was struggling to come to with a concrete suggestion for how to reword these notices :/
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
(The whatsnew and NEWS entries look great btw, it's just the notices in the API docs that feel slightly clunky to me)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I feel like ideally this notice would be phrased along the lines of "X behaviour/practice is now deprecated" rather than "a DeprecationWarning is now emitted". That would be more to-the-point, and it would also work more naturally with this directive (X behaviour/practice is deprecated in 3.12, and will be removed entirely in 3.14).
Yes, but documented as deprecated and emitting a DeprecationWarning are similar, but not equal, things 🙂 With the former, we don't need to emit a warning in the code.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
To your original question, btw: I think it's pretty standard to use .. deprecated or .. deprecated-removed, even if it's just a particular usage of an API, rather than the API itself. See e.g. https://docs.python.org/3.12/library/asyncio-policy.html#asyncio.DefaultEventLoopPolicy, where the directive is used even though the class itself hasn't been deprecated at all; or #19867, which deprecated just a specific parameter; or lots of other examples in our docs 🙂
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Aight! I'll try to reword it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I've updated it to use deprecated-removed, and I put in an extra line regarding what happens in 3.14. I'm too tired to reword the text 🙂
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The docs LGTM!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.