Skip to content

Conversation

chirizxc
Copy link

@chirizxc chirizxc commented Sep 10, 2025

@python-cla-bot
Copy link

python-cla-bot bot commented Sep 10, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

Comment on lines 633 to 636

:param str row:
:param int row:
The name of the row where the blob is located.

Choose a reason for hiding this comment

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

Is it really a name if it is an int?

Copy link
Author

Choose a reason for hiding this comment

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

"Row id of the row that contains the blob." how about this wording?

Copy link
Author

Choose a reason for hiding this comment

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

The source code of the sqlite3 module says it is "Row index."

Choose a reason for hiding this comment

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

I would probably go with "The index of the row where the blob is located." as the minimal semantically correct change.

But i am not a core dev. So my ideas are just suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer "The index of [...]".

@Eclips4 Eclips4 linked an issue Sep 10, 2025 that may be closed by this pull request
@picnixz
Copy link
Member

picnixz commented Sep 10, 2025

Err.... Actually... I'm very sorry but I think we should use the term "id". Here what I mean. The sqlite docs (https://sqlite.org/c3ref/blob_open.html) say ROWID (https://sqlite.org/lang_createtable.html#rowid). I think it's more precise than index because nothing guarantees that the first row has index 1 and there are tables without indices.

For instance in https://sqlite.org/queryplanner.html#_lookup_by_rowid, the rowId does not match the "index" if we were to consider the table as an array. So maybe ID is better (and the C code could be improved instead).

cc @erlend-aasland

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review docs Documentation in the Doc dir skip news
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

wrong type of row in sqlite3.Connection.blobopen
3 participants