Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve sqlite3 documentation of the new autocommit attribute #99435

Closed
wants to merge 9 commits into from

Conversation

geryogam
Copy link
Contributor

@geryogam geryogam commented Nov 13, 2022

Issue #99670.

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

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

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

geryogam commented Nov 22, 2022

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 geryogam marked this pull request as ready for review November 22, 2022 00:29
@erlend-aasland
Copy link
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
Copy link
Contributor Author

geryogam commented Nov 22, 2022

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.

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