Improve sqlite3 documentation of the new autocommit attribute

[geryogam]

Documentation

After reviewing the design and implementation of sqlite3’s new autocommit attribute of PR #93823, here is the follow-up review of the documentation, addressing 9 issues with PR #99435:

• Remove an unnecessary lambda in tests
• Clarify that implicit open or roll back happens in the same method call
• Add connect() to the list of implicit-transaction-opening methods
• Recommend commit() and rollback() in legacy manual commit mode
• Clarify PEP-249-compliant vs legacy transaction control
• Improve wording consistency
• Remove two ambiguous ‘underlying’
• Normalize ‘open transaction’
• Normalize ’transaction control’

[erlend-aasland]

See comment on the PR

[erlend-aasland]

Answering #99435 (comment):

To me the 5 first commits are the most important as they add new information.

• Remove an unnecessary lambda in tests

The first commit is a code style issue in the tests; it does not add new information; it does not alter the semantics of the tests or code in question. I agree it is a bit of a code smell, but it is not worth fixing just because it's code smell. If (or when) we touch similar parts of the code, for example because we are adding new tests, or because we are adjusting incorrect tests, we can sneak in such a style change. (Also, this change has nothing to do with the issue title.)

• Clarify that implicit open or roll back happens in the same method call

These sentences are part of the reference for cursor methods. The reference for cursor methods describe what happens when you invoke these methods. I'm not sure explicitly stating that for every method is worth it, and I believe it clutters the docs.

• Add connect() to the list of implicit-transaction-opening methods

I'm not sure this change is needed either. The docs say a transaction is always open; this implies a connection is open when the connection is created. Otherwise that sentence would have been incorrect.

Also, the current wording starts off simply by stating what behaviour the user can expect: A transaction is always open. Then it goes on adding details about the two most important methods, commit and rollback. The rewording in d864934 starts off by explaining the details and ends with explaining the behaviour. I'm not sure that is a good rephrasing. Also, the commit title does not reflect the actual change: it does not add connect to a list of transaction-opening methods; it does a complete rewording of a paragraph in addition to mentioning connect().

• Recommend commit() and rollback() in legacy manual commit mode

commit() and rollback() is already recommended in legacy commit mode; why is this needed? Rewording this again seems like churn to me.

• Clarify PEP-249-compliant vs legacy transaction control

IMO, this is not at all clarifying. PEP-249 does not describe an autocommit (non-transactional) mode. All it says is: some databases has some kind of concept of autocommit. If your underlying database supports such functionality, you can enable it when autocommit is True. PEP-249 leaves autocommit to the underlying database, and so do we. I'm strongly -1 to this change.

That is what I did by splitting this PR into 9 commits.

Unfortunately, that does not help; discussing and reviewing 9 separate issues in a single PR/issue is not productive. It is difficult to discuss (cross-conversations), and it is impossible to review, hence my closing the PR and the issue. If you find issues, create instead targeted and narrow issues, so we can discuss each change isolated.

Now for the rest of the commits:

• Improve wording consistency

Which inconsistency is this? I cannot deduce this inconsistency from the commit title "improve wording consistency". Looking at the commit diff, it makes many inconsistent changes; some stylish are rewordings and some are incorrect plural/singular adjustments. I don't see any explanation or comment regarding what's wrong with the current wording.

• Remove two ambiguous ‘underlying’

Ditto

• Normalize ‘open transaction’

Such a change might be worth considering, however see Guido's recent response to a similar concern.

• Normalize ’transaction control’

IMO, this change is stylish churn.

I do appreciate you being interested in improving the docs, but please try to identify and explain the problems with the current docs, preferrably one by one, instead of proposing multiple unrelated changes like this.

[geryogam]

Thanks for taking the time to consider each suggested changes.

• Remove an unnecessary lambda in tests

The first commit is a code style issue in the tests; it does not add new information; it does not alter the semantics of the tests or code in question. I agree it is a bit of a code smell, but it is not worth fixing just because it's code smell. If (or when) we touch similar parts of the code, for example because we are adding new tests, or because we are adjusting incorrect tests, we can sneak in such a style change. (Also, this change has nothing to do with the issue title.)

Yes this one was unrelated to this PR, but I was not sure it was worth opening a separate PR for such a trivial change.

• Clarify that implicit open or roll back happens in the same method call

These sentences are part of the reference for cursor methods. The reference for cursor methods describe what happens when you invoke these methods. I'm not sure explicitly stating that for every method is worth it, and I believe it clutters the docs.

I find the current wording a bit confusing, but that is just my interpretation.

• Add connect() to the list of implicit-transaction-opening methods

I'm not sure this change is needed either. The docs say a transaction is always open; this implies a connection is open when the connection is created. Otherwise that sentence would have been incorrect.

Also, the current wording starts off simply by stating what behaviour the user can expect: A transaction is always open. Then it goes on adding details about the two most important methods, commit and rollback. The rewording in d864934 starts off by explaining the details and ends with explaining the behaviour. I'm not sure that is a good rephrasing. Also, the commit title does not reflect the actual change: it does not add connect to a list of transaction-opening methods; it does a complete rewording of a paragraph in addition to mentioning connect().

Implicit-transaction-opening methods connect(), commit(), and rollback() are part of MAL’s specification so I expect to see them in the documentation. To me it is better to be explicit about them rather than relying on user’s deduction for connect() which is not obvious at all (remember that sqlite3 uses SQL parsing for implicitly opening transactions before DML statements in legacy manual commit mode). And I would even say it is the most important novelty of your PR.

• Recommend commit() and rollback() in legacy manual commit mode

commit() and rollback() is already recommended in legacy commit mode; why is this needed? Rewording this again seems like churn to me.

My bad, it is indeed a rewording and I should have put it in the ‘Improve wording consistency’ commit.

• Clarify PEP-249-compliant vs legacy transaction control

IMO, this is not at all clarifying. PEP-249 does not describe an autocommit (non-transactional) mode. All it says is: some databases has some kind of concept of autocommit. If your underlying database supports such functionality, you can enable it when autocommit is True. PEP-249 leaves autocommit to the underlying database, and so do we. I'm strongly -1 to this change.

So what you call ‘PEP-249 compliant’ is the manual commit mode specified in the PEP (implicit-transaction opening methods connect(), commit, and rollback()), not the auto-commit mode which is not specified in the PEP and depends on the underlying database engine.

Then I think that the sentence ‘This attribute controls :pep:249-compliant transaction behaviour.’ (at line 1257) is confusing as in addition to controlling PEP-249-compliant manual commit mode (when autocommit=False), it also controls auto-commit mode (when autocommit=True) which is not particularly PEP-249-compliant since PEP 249 only specifies manual commit mode, and it also controls both legacy non-PEP-249-compliant manual commit mode and legacy auto-commit mode (when autocommit=sqlite3.LEGACY_TRANSACTION_CONTROL). So what about replacing the sentence with ‘This attribute controls transaction behaviour.’?

Likewise for the sentence ‘Pre-Python 3.12 (non-:pep:249-compliant) transaction control.’ (at line 1270) which is confusing since in addition to controlling legacy non-PEP-249-compliant manual commit mode (when isolation_level='DEFERRED'), it also controls legacy auto-commit mode (when isolation_level=None) which is not particularly non-PEP-249-compliant since PEP 249 only specifies manual commit mode. So what about replacing the sentence with ‘Legacy (pre-Python 3.12) transaction control.’?

[erlend-aasland]

I'll try to explain this again, hopefully less ambiguous this time:

I find the current wording a bit confusing, but that is just my interpretation.

I understand that you find this confusing, hence your wish to change it. But I cannot know what you find confusing unless you tell me. That is why I've asked you to describe the problems you are seeing; unless you do so, all I can do is to guess. If you describe the issues you are seeing, it will be easier for us to be on the same page, and it is easier to have a fruitful conversation about how to improve things.

Also, as I already warned about, using a single GitHub issue to manage multiple unrelated issues is counter-productive; each post constains multiple unrelated paragraphs. Responding to each others posts quickly becomes very time-consuming, and it is difficult to follow the conversation history. This is why I've recommended to create narrow and targeted issues where you describe each problem on its own, and I now strongly recommend this again, but I suggest you take a look at the devguide first:

• https://devguide.python.org/triage/issue-tracker/#reporting-an-issue

Let me quote from the devguide link:

[...] in the [issue] Title field, enter a very short description of the problem; less than ten words is good;

[...] in the [issue] Write field, describe the problem in detail using hints from the template that was put in that field for you [...]

Related: I see that the "Making Good PRs" section in the devguide unfortunately does not (yet) mention that a PR should be focused and narrowed down to a single change; it should not try to fix many things at once, as that makes it hard to review.