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

bpo-38413: Improve documentation of sqlite3 module #23159

Merged
merged 5 commits into from
Jan 6, 2021

Conversation

greatvovan
Copy link
Contributor

@greatvovan greatvovan commented Nov 5, 2020

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

Remove outdate note about multithreading in sqlite3
Improve wording and formatting in the footnote.
@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
Copy link
Contributor Author

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

@dimaqq
Copy link
Contributor

dimaqq commented Nov 5, 2020

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

AFAIK, someone has to do that for you.

@github-actions
Copy link

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

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

dimaqq commented Dec 16, 2020

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!

@greatvovan
Copy link
Contributor Author

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

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Dec 17, 2020
@greatvovan
Copy link
Contributor Author

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

@greatvovan
Copy link
Contributor Author

@willingc please help.

@willingc
Copy link
Contributor

willingc commented Jan 5, 2021

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

@willingc willingc self-requested a review January 5, 2021 00:54
Doc/library/sqlite3.rst Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

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.

Revert to mentioning "configure" script.
Keep original lines
@greatvovan
Copy link
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".

@greatvovan
Copy link
Contributor Author

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

@berkerpeksag
Copy link
Member

berkerpeksag commented Jan 5, 2021

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."

Revert back some changes unrelated to the BPO.
Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
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

@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 6, 2021
@berkerpeksag
Copy link
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.

berkerpeksag pushed a commit that referenced this pull request Jan 6, 2021
@greatvovan
Copy link
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.

@greatvovan
Copy link
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?

@berkerpeksag
Copy link
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 added a commit to greatvovan/cpython that referenced this pull request Jan 6, 2021
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jan 6, 2021
@greatvovan
Copy link
Contributor Author

greatvovan commented Jan 6, 2021

@berkerpeksag #24145

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

berkerpeksag pushed a commit that referenced this pull request Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants