Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-100414: Add SQLite backend to dbm#114481

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
erlend-aasland merged 48 commits intopython:mainfromerlend-aasland:sqlite/dbm
Feb 14, 2024

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedJan 23, 2024
edited
Loading

@erlend-aasland
Copy link
ContributorAuthor

cc.@presidento

@erlend-aasland
Copy link
ContributorAuthor

I suggest to focus this PR on the basic, required functionalityonly. We can optimise things in follow-up PRs.

@erlend-aaslanderlend-aasland marked this pull request as ready for reviewJanuary 23, 2024 22:51
@erlend-aasland
Copy link
ContributorAuthor

I think the basic functionality is ready for an initial round of reviews.

Tests

There's a basic test suite inLib/test_dbm.py, and I also added somedbm.sqlite3 specific tests inLib/test_dbm_sqlite3.py. We can expand the latter in follow-up PRs.

Docs

I also added som basic docs; unless significant information is missing, I suggest we work on the docs in follow-up PRs.

@erlend-aasland
Copy link
ContributorAuthor

I made acore dev poll on Discourse regarding if the SQLite backend should be the default one; 16 out of 21 was in favour (Skip changed his mind post voting). This PR doesnot make the SQLite backend the default. If we want to change the default, we can do it post merging the feature itself. Another possibility,suggested by @ethanfurman:

Shouldn’t we make it the default now so it can be tested in the alphas? We can undo the default portion if it turns out to be a problem.

@serhiy-storchaka, should we land this?

@serhiy-storchaka
Copy link
Member

It seems that currently it is not compatible with other dbm implementations. They all are bytes oriented, but accept strings as input (it is a Python 2 legacy). Most of the dbm tests use string input, so I expect that many user code does it too. But the output always expected to be bytes.

Supporting other types, supported by Sqlite looks attractive, but we implement a backend to dbm, so it should be compatible. We can later add an option to alter the behavior, but it should not be default.

@erlend-aasland
Copy link
ContributorAuthor

It seems that currently it is not compatible with other dbm implementations. They all are bytes oriented, but accept strings as input (it is a Python 2 legacy). Most of the dbm tests use string input, so I expect that many user code does it too. But the output always expected to be bytes.

Supporting other types, supported by Sqlite looks attractive, but we implement a backend to dbm, so it should be compatible. We can later add an option to alter the behavior, but it should not be default.

As discussed in DM, I agree with this. I addressed your concern ine782fad.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM if all operations still accept strings.

# (raw, coerced)
(42, b"42"),
(3.14, b"3.14"),
("string", b"string"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Does it work with non-ASCII strings?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do you mean such a test?

diff --git a/Lib/test/test_dbm_sqlite3.py b/Lib/test/test_dbm_sqlite3.pyindex 7bc2a03035..3d2dc5fd55 100644--- a/Lib/test/test_dbm_sqlite3.py+++ b/Lib/test/test_dbm_sqlite3.py@@ -214,6 +214,7 @@ class DataTypes(_SQLiteDbmTests):         (3.14, b"3.14"),         ("string", b"string"),         (b"bytes", b"bytes"),+        ("æøå", b"\xc3\xa6\xc3\xb8\xc3\xa5"),     )      def setUp(self):

@erlend-aasland
Copy link
ContributorAuthor

With34930cb, the dbm.sqlite3 backend now aligns with the other backends; for example, the general code snippet in the dbm docs now works as expected with the new backend.

@erlend-aasland
Copy link
ContributorAuthor

Let's land this,@serhiy-storchaka. There is plenty of opportunities to tweak the implementation and to expand the test suite.

Thanks@smontanaro for the initial idea,@rhettinger for the initial implementation, and everyone else involved for ideas, reviews and critique. Thanks to@serhiy-storchaka for the, as always, extremely thorough review of the feature and the implementation.

@erlend-aaslanderlend-aaslandenabled auto-merge (squash)February 14, 2024 11:02
@erlend-aaslanderlend-aasland merged commitdd5e4d9 intopython:mainFeb 14, 2024
@erlend-aaslanderlend-aasland deleted the sqlite/dbm branchFebruary 14, 2024 11:14
@bedevere-bot

This comment was marked as resolved.

@bedevere-bot

This comment was marked as duplicate.

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commentedFeb 14, 2024
edited
Loading

⚠️⚠️⚠️ Buildbot failure⚠️⚠️⚠️

[...]

Failed tests:

  • test_dbm_sqlite3

[...]
ModuleNotFoundError: No module named '_sqlite3'

Proposed fixes:

@bedevere-bot

This comment was marked as duplicate.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull requestFeb 14, 2024
@bedevere-bot

This comment was marked as resolved.

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull requestFeb 14, 2024
Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@gpsheadgpsheadgpshead left review comments

@presidentopresidentopresidento left review comments

@felixxmfelixxmfelixxm left review comments

@corona10corona10corona10 approved these changes

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@rhettingerrhettingerAwaiting requested review from rhettinger

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksagberkerpeksag is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@erlend-aasland@corona10@serhiy-storchaka@bedevere-bot@gpshead@presidento@felixxm

[8]ページ先頭

©2009-2025 Movatter.jp