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

bpo-42064: Migrate tosqlite3_create_collation_v2#27156

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

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedJul 15, 2021
edited
Loading

As a nice side effect, we can now remove the collation dict from the
connection object, since SQLite now takes care of destroying the
callback context.

This change is needed in order to make it more convenient to add a
module state to thesqlite3 module.

https://bugs.python.org/issue42064

This implies that SQLite now takes care of destroying the callbackcontext (the PyObject callable it has been passed), so we can strip thecollation dict from the connection object.
@erlend-aasland
Copy link
ContributorAuthor

sqlite3_create_collation andsqlite3_create_collation_v2 are described here:https://sqlite.org/c3ref/create_collation.html

@erlend-aasland
Copy link
ContributorAuthor

FYI, the test suite already exercise clearing a collation callback and re-registering a collation callback.

@erlend-aasland
Copy link
ContributorAuthor

BTW, I'd like to change thefinally label at the end ofpysqlite_connection_init_impl toerror, for consistency with the rest of the code base.

Copy link
Member

@encukouencukou left a comment

Choose a reason for hiding this comment

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

Looks good so far!

I see that the beginning of the function does some busywork convertingname to uppercase and checking for allowed characters. Is that limitation there only to make that sure comparison of keys in thecollations dict matchsqlite3_strnicmp? If so,PyUnicode_AsUTF8(name) could be used instead.

As for thefinallyerror rename, I'd sayfinally is a better name when it's also executed in the non-error case...

@erlend-aasland
Copy link
ContributorAuthor

I see that the beginning of the function does some busywork convertingname to uppercase and checking for allowed characters. Is that limitation there only to make that sure comparison of keys in thecollations dict matchsqlite3_strnicmp? If so,PyUnicode_AsUTF8(name) could be used instead.

Yes, I noticed that part as well. I haven't investigated why this was added in the first place. I'll try withPyUnicode_AsUTF8(). Thanks!

As for thefinallyerror rename, I'd sayfinally is a better name when it's also executed in the non-error case...

True.

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commentedJul 20, 2021
edited
Loading

FYI, the collation code (including the uppercase conversion / check), was added topysqlite in 2006, when it still was an external project. The code seems to have been borrowed directly fromapsw.

UPDATE:
SQLite accepts UTF8 collation names; there should be no reason to limit collation names to ASCII. Maybe there was a bug or a limitation in earlier SQLite versions. Theapsw code has removed this limitation and now accepts UTF8 names. We can do that as well. I'll open up an issue for that.

UPDATE AGAIN:
I've createdissue 44688.I'll open a PR once this is merged. PR#27395 opened.

@encukouencukou merged commit890e229 intopython:mainJul 27, 2021
@encukou
Copy link
Member

Thank you!

erlend-aasland reacted with hooray emoji

@erlend-aaslanderlend-aasland deleted the sqlite-use-create-collation-v2 branchJuly 27, 2021 21:30
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull requestJul 27, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksagberkerpeksag is a code owner

@pablogsalpablogsalAwaiting requested review from pablogsal

@encukouencukouAwaiting requested review from encukou

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland@encukou@the-knights-who-say-ni@bedevere-bot

[8]ページ先頭

©2009-2026 Movatter.jp