FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Improve sqlite3 documentation of the new autocommit attribute by geryogam · Pull Request #99435 · python/cpython · GitHub

Repository navigation

Improve sqlite3 documentation of the new autocommit attribute - #99435

Closed
geryogam wants to merge 9 commits into
python:mainfrom
geryogam:patch-14
Closed

geryogam wants to merge 9 commits into
python:mainfrom
geryogam:patch-14

Conversation

geryogam commented Nov 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Issue #99670.

geryogam force-pushed the patch-14 branch 4 times, most recently from dad8b33 to 85e7a93 Compare November 13, 2022 22:44
geryogam marked this pull request as ready for review November 13, 2022 23:32

Copy link
Copy Markdown
Contributor

Please open an issue so we can discuss the problems you've identified first.

erlend-aasland marked this pull request as draft November 14, 2022 08:24
geryogam changed the title Follow-up to PR #93823 Improve sqlite3 documentation of the new autocommit attribute Nov 22, 2022

geryogam commented Nov 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Please open an issue so we can discuss the problems you've identified first.

@erlend-aasland I have opened issue #99670.

I have further subdivided this PR in 9 logical commits instead of 6 for better isolation of the problems and easier review.

geryogam marked this pull request as ready for review November 22, 2022 00:29

Copy link
Copy Markdown
Contributor

I have opened issue #99670.

I have further subdivided this PR in 9 logical commits instead of 6 for better isolation of the problems and easier review.

This issue just lists the summary of the various commits in this PR; it does not describe any issues at all. A lot of these commits are changes completely unrelated to the PR title (for example normalising the use of "open transaction" versus "pending transaction"). Some of these changes might be worth doing. A lot of them are IMO unneeded churn. I suggest closing this PR and the accompanying issue. Try instead to isolate each problem, describe it accurately and explain why it is a problem. Then it will be possible to discuss the issue, decide if it is a real problem, and what the suggested solution should be.

geryogam commented Nov 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Some of these changes might be worth doing.

Could you list them please? To me the 5 first commits are the most important as they add new information. The 4 remaining ones are just rephrasing so a matter of opinion.

Try instead to isolate each problem,

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

describe it accurately and explain why it is a problem.

That is what I did with the commit messages. At this point I am not sure what more I can add so I kindly suggest that you list the commits that you agree with, otherwise we will not make any progress.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL