GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-38413: Improve documentation of sqlite3 module by greatvovan · Pull Request #23159 · python/cpython

Conversation

@greatvovan

@greatvovan greatvovan commented Nov 5, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This removes an outdated section in the documentation and slightly improves wording and formatting.

See https://bugs.python.org/issue38413

https://bugs.python.org/issue38413

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
greatvovan added 2 commits October 28, 2020 00:55
@greatvovan
Fix bpo-38413
cf0436d
Remove outdate note about multithreading in sqlite3
@greatvovan
Update sqlite3.rst
cb23459
Improve wording and formatting in the footnote.
@greatvovan
greatvovan requested a review from berkerpeksag as a code owner November 5, 2020 02:00
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 5, 2020
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Nov 5, 2020
dimaqq
dimaqq approved these changes Nov 5, 2020
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Nov 5, 2020
@greatvovan
greatvovan marked this pull request as draft November 5, 2020 02:11
@greatvovan
greatvovan marked this pull request as ready for review November 5, 2020 02:14
@greatvovan

greatvovan commented Nov 5, 2020

Copy link
Copy Markdown
Contributor Author

Sorry, I am new to this repo, how can I assign "skip news" label?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dimaqq

dimaqq commented Nov 5, 2020

Copy link
Copy Markdown
Contributor

Sorry, I am new to this repo, how can I assign "skip news" label?

AFAIK, someone has to do that for you.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions

github-actions Bot commented Dec 16, 2020

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 16, 2020
@dimaqq

dimaqq commented Dec 16, 2020

Copy link
Copy Markdown
Contributor

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

No, it's not!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@greatvovan

greatvovan commented Dec 16, 2020

Copy link
Copy Markdown
Contributor Author

I am the author. Is anything else required from me? Any concerns?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Dec 17, 2020
@greatvovan

greatvovan commented Dec 18, 2020

Copy link
Copy Markdown
Contributor Author

Anybody? @berkerpeksag, it shows you as code owner... Is there anything that needs to be fixed?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@greatvovan

greatvovan commented Dec 30, 2020

Copy link
Copy Markdown
Contributor Author

@willingc please help.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@willingc

willingc commented Jan 5, 2021

Copy link
Copy Markdown
Contributor

@greatvovan I've been offline for a few weeks. I'll add this to my queue for review. Thanks for your patience.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@willingc
willingc self-requested a review January 5, 2021 00:54
berkerpeksag
berkerpeksag requested changes Jan 5, 2021
View reviewed changes
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting core review label Jan 5, 2021
@bedevere-bot

bedevere-bot commented Jan 5, 2021

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting changes label Jan 5, 2021
greatvovan added 2 commits January 5, 2021 09:00
@greatvovan
Update sqlite3.rst
ced42cd
Revert to mentioning "configure" script.
@greatvovan
Update sqlite3.rst
9a04f52
Keep original lines
@greatvovan

greatvovan commented Jan 5, 2021

Copy link
Copy Markdown
Contributor Author

Thanks @berkerpeksag, I reverted to mentioning configure script and added the link you provided. I insist that the original sentence is bad as it gives no hints to the topic 90% of Python developers will not ever touch.

Please also note that the main issue here is removing a paragraph that is inconsistent with other article sections (see BPO), it is not a "random formatting change".

willingc reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@greatvovan
greatvovan requested a review from berkerpeksag January 5, 2021 17:11
@greatvovan

greatvovan commented Jan 5, 2021

Copy link
Copy Markdown
Contributor Author

I think it worths to mention that Python must be rebuilt (recompiled) as just running the script will do nothing.

willingc reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@berkerpeksag

berkerpeksag commented Jan 5, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Let me try again. You are trying to make two separate changes in a single PR:

  1. Removing an outdated section in the documentation. This is completely fine and I'd be happy to merge such PR.
  2. Making random formatting and wording changes on a completely unrelated sentence explaining something you don't fully understand. This is not okay for the following reasons:
    • We prefer doing one change (this can be a new feature, bug fix, documentation changes) per BPO issue. bpo-38413 is about removing an outdated section in the sqlite3 documentation. You cannot include another change. It's a bad practice. If you found something else while working on bpo-38413, the best practice is to open a new BPO issue or submit a PR (for example, if you found a typo, you can just submit a separate PR without creating an issue)
    • The change you are trying to insist on including is incorrect and the old sentence hasn't been caused any confusion for the past decade. So I'm not going to change it just because someone who initially proposed replacing configure with pyenv's PYTHON_CONFIGURE_OPTS said so.
    • It's okay to not know about a very niche feature. We all learn something new all the time. However, it's not okay to keep pushing for it when someone else said "no your change is incorrect, please revert to the old version."
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@greatvovan
Update sqlite3.rst
c6e5f91
Revert back some changes unrelated to the BPO.
berkerpeksag
berkerpeksag approved these changes Jan 6, 2021
View reviewed changes

@berkerpeksag berkerpeksag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Jan 6, 2021
@berkerpeksag berkerpeksag added needs backport to 3.8 skip news labels Jan 6, 2021
@berkerpeksag
berkerpeksag merged commit f9949f8 into python:master Jan 6, 2021
@miss-islington

miss-islington commented Jan 6, 2021

Copy link
Copy Markdown
Contributor

Thanks @greatvovan for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting merge label Jan 6, 2021
@miss-islington

miss-islington commented Jan 6, 2021

Copy link
Copy Markdown
Contributor

Sorry @greatvovan and @berkerpeksag, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker f9949f82e17c88609adb53eff3a7d5cd63a645bd 3.9

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington miss-islington assigned berkerpeksag Jan 6, 2021
@bedevere-bot

bedevere-bot commented Jan 6, 2021

Copy link
Copy Markdown

GH-24134 is a backport of this pull request to the 3.8 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.8 label Jan 6, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 6, 2021
@greatvovan @miss-islington
bpo-38413: Remove outdated section about multithreading in sqlite3 (p…
a7a06a1
…ythonGH-23159)

(cherry picked from commit f9949f8)

Co-authored-by: Vladimir <[email protected]>
@berkerpeksag

berkerpeksag commented Jan 6, 2021

Copy link
Copy Markdown
Member

@greatvovan do you have time to submit a backport PR for 3.9? I can help you if you don't want to use cherry_picker.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

berkerpeksag pushed a commit that referenced this pull request Jan 6, 2021
@miss-islington @greatvovan
bpo-38413: Remove outdated section about multithreading in sqlite3 (G…
03a079f
…H-23159)

(cherry picked from commit f9949f8)

Co-authored-by: Vladimir <[email protected]>
@greatvovan

greatvovan commented Jan 6, 2021

Copy link
Copy Markdown
Contributor Author
  • I admitted that I was incorrect in my initial change. With your help it was fixed, this is why we have review process of pull requests. I am still not getting what is wrong with the second version and why you are still saying I don't understand something two times in one post.
  • I got your point about creating a separate BPO/PR for other unrelated issues and am actually agree that this is better, but in this case I believe it's easier to just inform your counterpart about best practices for the future and proceed. Due to such kind of bureaucracy you have 1.5K of open pull requests.
  • You have no data to state that "the old sentence hasn't been caused any confusion for the past decade". Not everyone who finds a confusing phrase runs submitting a change. Questions about enabling sqlite extensions can be found on StackOverflow and in other places, so it is indeed calls confusion.
  • I reverted the original sentence. You just left the Python documentation with this ugly phrase:

you must pass --enable-loadable-sqlite-extensions to configure

My congratulations.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@greatvovan

greatvovan commented Jan 6, 2021

Copy link
Copy Markdown
Contributor Author

do you have time to submit a backport PR for 3.9?

Yes, I do have time, but your help is appreciated (links, recommendations). Isn't it just a PR to another branch with the same change?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@berkerpeksag

berkerpeksag commented Jan 6, 2021

Copy link
Copy Markdown
Member

Isn't it just a PR to another branch with the same change?

Yes, you basically do:

$ git checkout -b backport-to-3.9 3.9
$ git cherry-pick -x f9949f82e17c88609adb53eff3a7d5cd63a645bd 

And add [3.9] prefix to PR title when submitting it. You can copy and paste the title of GH-24134 and replace 3.8 with 3.9.

greatvovan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

greatvovan added a commit to greatvovan/cpython that referenced this pull request Jan 6, 2021
@greatvovan
bpo-38413: Remove outdated section about multithreading in sqlite3 (p…
e842387
…ythonGH-23159)

(cherry picked from commit f9949f8)
@bedevere-bot

bedevere-bot commented Jan 6, 2021

Copy link
Copy Markdown

GH-24145 is a backport of this pull request to the 3.9 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Jan 6, 2021
@greatvovan

greatvovan commented Jan 6, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@berkerpeksag #24145

Edit: oh, the bot is faster than me...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

berkerpeksag pushed a commit that referenced this pull request Jan 6, 2021
@greatvovan
bpo-38413: Remove outdated section about multithreading in sqlite3 (G…
49c150f
…H-23159)

(cherry picked from commit f9949f8)
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@greatvovan @adorilson
bpo-38413: Remove outdated section about multithreading in sqlite3 (p…
082a0a6
…ythonGH-23159)
@erlend-aasland erlend-aasland mentioned this pull request Jul 27, 2022
Improve the sqlite3 reference #95273
Closed
5 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes
@willingc willingc Awaiting requested review from willingc
+1 more reviewer
@dimaqq dimaqq dimaqq approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@berkerpeksag berkerpeksag

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@greatvovan @dimaqq @willingc @bedevere-bot @berkerpeksag @miss-islington @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.