Conversation

erlend-aasland

@erlend-aasland

cc. @presidento

@erlend-aasland

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

@erlend-aaslanderlend-aasland marked this pull request as ready for review January 23, 2024 22:51
@erlend-aasland

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

Tests

There's a basic test suite in Lib/test_dbm.py, and I also added some dbm.sqlite3 specific tests in Lib/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

I made a core 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 does not 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

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

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 in e782fad.

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
Contributor Author

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.py
index 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

With 34930cb, 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

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-aasland enabled auto-merge (squash) February 14, 2024 11:02
@erlend-aaslanderlend-aasland merged commit dd5e4d9 into python:main Feb 14, 2024
@erlend-aaslanderlend-aasland deleted the sqlite/dbm branch February 14, 2024 11:14
@bedevere-bot

This comment was marked as resolved.

@bedevere-bot

This comment was marked as duplicate.

@erlend-aasland

⚠️⚠️⚠️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 request Feb 14, 2024
@bedevere-bot

This comment was marked as resolved.

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
Co-authored-by: Raymond Hettinger <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Mariusz Felisiak <[email protected]>
Sign up for free to join this conversation on . Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.