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

Jump to bottom

gh-93370 : Deprecate sqlite3.version and sqlite3.version_info #93482

Merged
merged 29 commits into from Jun 7, 2022
Merged

gh-93370 : Deprecate sqlite3.version and sqlite3.version_info #93482

merged 29 commits into from Jun 7, 2022
+50 −2

Conversation

rawwar
Copy link
Contributor

@rawwar rawwar commented Jun 3, 2022

removed sqlite3.version and sqlite3.version_info

@cpython-cla-bot
Copy link

@cpython-cla-bot cpython-cla-bot bot commented Jun 3, 2022 •
edited

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 3, 2022

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@rawwar
Copy link
Contributor Author

@rawwar rawwar commented Jun 3, 2022 •
edited

closes #93370

@rawwar
Copy link
Contributor Author

@rawwar rawwar commented Jun 3, 2022

I think what I did is wrong. I should be deprecating instead of removing in current version. But, after thinking about how to deprecate sqlite3.version_info and sqlite3.version I don't understand how exactly I can do this. I saw several other deprecation PR's and all of them are either methods or class attributes. That makes it easy to raise a deprecation warning. But, here, version comes from _sqlite as a string constant. Not exactly sure how to display warning when calling an variable.

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jun 3, 2022 •
edited

Not exactly sure how to display warning when calling an variable.

Take a look at https://peps.python.org/pep-0562/

Example usage for a DeprecationWarning:

cpython/Lib/io.py

Line 60 in 9b12b1b

def __getattr__(name):

@rawwar
Copy link
Contributor Author

@rawwar rawwar commented Jun 3, 2022 •
edited

@AlexWaygood , can you please check my usage of __getattr__

AlexWaygood
AlexWaygood reviewed Jun 3, 2022
View changes
Lib/sqlite3/__init__.py Outdated Show resolved Hide resolved
Lib/sqlite3/__init__.py Outdated Show resolved Hide resolved
Lib/sqlite3/dbapi2.py Outdated Show resolved Hide resolved
Lib/sqlite3/dbapi2.py Outdated Show resolved Hide resolved
Lib/sqlite3/dbapi2.py Outdated Show resolved Hide resolved
@rawwar
Copy link
Contributor Author

@rawwar rawwar commented Jun 4, 2022

@AlexWaygood , Thank you so much for your feedback. I've incorporated your feedback

@rawwar rawwar requested a review from AlexWaygood Jun 4, 2022
AlexWaygood
AlexWaygood reviewed Jun 4, 2022
View changes
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Thanks! This is starting to look pretty good. However, it's still missing:

  • Tests
  • Updates to the documentation

Could you add those, please? 🙂 Take a look at #23064 as an example of a previous PR deprecating features — it should make clear the kinds of changes that you need to make to the test files and the docs.

Since this is your first contribution to CPython, you're also very welcome to add your name to https://github.com/python/cpython/blob/main/Misc/ACKS if you want to, though that isn't compulsory!

Lib/sqlite3/__init__.py Outdated Show resolved Hide resolved
Lib/sqlite3/dbapi2.py Outdated Show resolved Hide resolved
Lib/sqlite3/dbapi2.py Outdated Show resolved Hide resolved
rawwar and others added 3 commits Jun 4, 2022
@rawwar @AlexWaygood
Update Lib/sqlite3/__init__.py
fa80dd6 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@rawwar @AlexWaygood
Update Lib/sqlite3/dbapi2.py
f47ddf3 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@rawwar @AlexWaygood
Update Lib/sqlite3/dbapi2.py
f033aea 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AA-Turner
AA-Turner suggested changes Jun 4, 2022
View changes
Copy link
Member

@AA-Turner AA-Turner left a comment •
edited

Some minor comments. As Alex said, you still need tests and to update the documentation.

A

Lib/sqlite3/dbapi2.py Outdated Show resolved Hide resolved
Lib/sqlite3/dbapi2.py Outdated Show resolved Hide resolved
Lib/sqlite3/__init__.py Show resolved Hide resolved
Lib/sqlite3/dbapi2.py Show resolved Hide resolved
Misc/NEWS.d/next/Library/2022-06-03-22-13-28.gh-issue-93370.tjfu9L.rst Outdated Show resolved Hide resolved
@rawwar
Copy link
Contributor Author

@rawwar rawwar commented Jun 4, 2022

@AlexWaygood , I reused comments of @erlend-aasland in #93370

The sqlite3.version and sqlite3.version_info attributes have historically created some confusion, as they are sometimes mistaken for the SQLite 3 version number. They used to reflect the pysqlite1 version number, until the external pysqlite package stopped upstreaming changes to the stdlib sqlite3 module2, but today they carry no meaning or practical value3.

In order to avoid future frustration, I suggest to deprecate sqlite3.version and sqlite3.version_info in 3.12, and remove them in 3.14.

AA-Turner
AA-Turner approved these changes Jun 4, 2022
View changes
Copy link
Member

@AA-Turner AA-Turner left a comment

Thanks for your work @rawwar!

A

AlexWaygood
AlexWaygood reviewed Jun 5, 2022
View changes
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Some documentation nits :)

Also, if you wanted to be super-thorough, you could also test that these also cause DeprecationWarning to be emitted:

from sqlite import version
from sqlite import version_info
from sqlite.dbapi2 import version
from sqlite.dbapi2 import version_info

Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
erlend-aasland and others added 2 commits Jun 5, 2022
@erlend-aasland @AlexWaygood
Update Doc/library/sqlite3.rst
81cf7fa 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@erlend-aasland @AlexWaygood
Update Doc/library/sqlite3.rst
964cf86 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AA-Turner
AA-Turner reviewed Jun 7, 2022
View changes
Copy link
Member

@AA-Turner AA-Turner left a comment

The line length of the tests is over 100 -- my suggestion doesn't bring it under 80, but makes it less egregious.

A

Lib/test/test_sqlite3/test_dbapi.py Outdated Show resolved Hide resolved
Lib/test/test_sqlite3/test_dbapi.py Outdated Show resolved Hide resolved
Lib/test/test_sqlite3/test_dbapi.py Outdated Show resolved Hide resolved
Lib/test/test_sqlite3/test_dbapi.py Outdated Show resolved Hide resolved
@rawwar @AA-Turner
Apply suggestions from code review
45478cc 
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
AlexWaygood
AlexWaygood approved these changes Jun 7, 2022
View changes
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Thanks!

erlend-aasland
erlend-aasland approved these changes Jun 7, 2022
View changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Looks good.

We haven't added warnings to sqlite3.dbapi2 before, only sqlite3 attributes, but it is ok; it will only stay for two versions :)

Also, I would probably have used subTest instead of spelling each test out.

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jun 7, 2022

Thanks for reviewing everyone, and thanks for the PR, Kalyan! I'm going to merge this as soon as the CI is green.

@erlend-aasland erlend-aasland merged commit ffc58a9 into python:main Jun 7, 2022
13 checks passed
@erlend-aasland erlend-aasland linked an issue Jun 7, 2022 that may be closed by this pull request
Deprecate sqlite3.version and sqlite3.version_info #93370
Closed
@erlend-aasland erlend-aasland mentioned this pull request Jun 7, 2022
Closed
@rawwar rawwar deleted the fix-issue-93370 branch Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland

@AA-Turner AA-Turner

@AlexWaygood AlexWaygood

@berkerpeksag berkerpeksag

Assignees
No one assigned
Labels
expert-sqlite3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate sqlite3.version and sqlite3.version_info
5 participants
@rawwar @bedevere-bot @AlexWaygood @AA-Turner @erlend-aasland