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 — gh-95273: Align sqlite3 const refs with the devguide by erlend-aasland · Pull Request #95525 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Aug 1, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: Improve the sqlite3 reference #95273

Sorry, something went wrong.

Uh oh!

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

All reactions
@erlend-aasland
pythongh-95273: Align sqlite3 const refs with the devguide
83104d4
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner August 1, 2022 09:25
@erlend-aasland erlend-aasland added needs backport to 3.10 needs backport to 3.11 only security fixes labels Aug 1, 2022
@erlend-aasland
erlend-aasland requested a review from ezio-melotti August 1, 2022 09:25
@bedevere-bot bedevere-bot added awaiting core review docs Documentation in the Doc dir skip news labels Aug 1, 2022
@erlend-aasland
fix table
31541ce
@erlend-aasland

erlend-aasland commented Aug 1, 2022

Copy link
Copy Markdown
Contributor Author

OTOH, adding ! to the existing refs will definitely look prettier, IMO. I will consider this.

All reactions

Sorry, something went wrong.

Uh oh!

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

ezio-melotti
ezio-melotti approved these changes Aug 3, 2022
View reviewed changes

@ezio-melotti ezio-melotti 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.

As discussed in python/devguide#916 (comment), using ``...`` is preferable to :const:`!...`.

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
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 3, 2022
@erlend-aasland

erlend-aasland commented Aug 3, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Thanks, Ezio! I forgot to change the parameter list types1; I'll update those before merging.

OTOH, I'm not sure about changing those; maybe best to leave those to the :type directives. Thoughts, @CAM-Gerlach, @ezio-melotti?

Footnotes

  1. Looking at the patch again, I see that I deliberately changed them. ↩

CAM-Gerlach 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.

@CAM-Gerlach

CAM-Gerlach commented Aug 3, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

OTOH, I'm not sure about changing those; maybe best to leave those to the :type directives. Thoughts, @CAM-Gerlach, @ezio-melotti?

That's what I would do (and have done in my PRs doing this elsewhere) as its simpler, consistent with standard Sphinxdoc usage and the structure makes its semantics unambiguous without the need for additional formatting, particularly since the context is as a type, not a code literal. If we need to customize it, we could monkeypatch the directive or submit an upstream PR instead of manually doing every one.

All reactions

Sorry, something went wrong.

Uh oh!

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

CAM-Gerlach
CAM-Gerlach approved these changes Aug 3, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach 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.

I regexed the doc, built it and and spot-checked the preview, and this LGTM, thanks @erlend-aasland

Sorry, something went wrong.

Uh oh!

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

All reactions
@erlend-aasland

erlend-aasland commented Aug 3, 2022

Copy link
Copy Markdown
Contributor Author

Yes, you spelled out my thoughts about this, CAM :)

Also, +1 to this:

If we need to customize it, we could monkeypatch the directive or submit an upstream PR instead of manually doing every one.

Thanks for the review, both of you. Highly appreciated! I'll land this.

CAM-Gerlach reacted with thumbs up emoji ezio-melotti reacted with hooray emoji CAM-Gerlach reacted with heart emoji
All reactions
  • 👍 1 reaction
  • 🎉 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
erlend-aasland merged commit 4d02572 into python:main Aug 3, 2022
@miss-islington

miss-islington commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

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 Aug 3, 2022
@erlend-aasland
erlend-aasland deleted the sqlite-reference/none-true-false branch August 3, 2022 20:21
@miss-islington

miss-islington commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

Sorry, @erlend-aasland, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 4d02572f8c39b16c83c0883917db4e31efc1048e 3.10

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.11 only security fixes label Aug 3, 2022
@bedevere-bot

bedevere-bot commented Aug 3, 2022

Copy link
Copy Markdown

GH-95616 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington miss-islington assigned erlend-aasland Aug 3, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 3, 2022
@erlend-aasland @miss-islington
pythongh-95273: Align sqlite3 const doc refs with the devguide recomm…
18eaa5c
…endations (pythonGH-95525)

(cherry picked from commit 4d02572)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
miss-islington added a commit that referenced this pull request Aug 3, 2022
@miss-islington @erlend-aasland
gh-95273: Align sqlite3 const doc refs with the devguide recommendati…
8d09626
…ons (GH-95525)

(cherry picked from commit 4d02572)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Aug 3, 2022
@erlend-aasland
[3.10] pythongh-95273: Align sqlite3 const doc refs with the devguide…
3c69446
… recommendations (pythonGH-95525).

(cherry picked from commit 4d02572)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Aug 3, 2022
@bedevere-bot

bedevere-bot commented Aug 3, 2022

Copy link
Copy Markdown

GH-95618 is a backport of this pull request to the 3.10 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@CAM-Gerlach

CAM-Gerlach commented Aug 3, 2022

Copy link
Copy Markdown
Member

Yes, you spelled out my thoughts about this, CAM :)

Probably less concisely than you would have, though 😆

erlend-aasland and ezio-melotti reacted with laugh emoji
All reactions
  • 😄 2 reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland added a commit that referenced this pull request Aug 3, 2022
@erlend-aasland
[3.10] gh-95273: Align sqlite3 const doc refs with the devguide recom…
ac005ea
…mendations (GH-95525). (#95618)

(cherry picked from commit 4d02572)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@erlend-aasland erlend-aasland mentioned this pull request Aug 3, 2022
Improve the sqlite3 reference #95273
Closed
5 tasks done
@erlend-aasland erlend-aasland linked an issue Aug 3, 2022 that may be closed by this pull request
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

@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes
@ezio-melotti ezio-melotti ezio-melotti approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Uh oh!

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

Assignees

@erlend-aasland erlend-aasland

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.

Improve the sqlite3 reference

Uh oh!

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

5 participants

@erlend-aasland @CAM-Gerlach @miss-islington @bedevere-bot @ezio-melotti
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.