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 — PEP 594: Apply typofixes & language tweaks by bskinn · Pull Request #2334 · python/peps

Conversation

@bskinn

@bskinn bskinn commented Feb 16, 2022

Copy link
Copy Markdown
Contributor

Just a few things I noticed on my read-through.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bskinn
Apply typofixes & language tweaks to PEP 594
9aac019
Just a few things I noticed on my read-through.
@bskinn
bskinn requested review from brettcannon and tiran as code owners February 16, 2022 19:50
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 16, 2022
CAM-Gerlach
CAM-Gerlach reviewed Feb 16, 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.

A few minor further tweaks, otherwise LGTM. Thanks @bskinn !

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread pep-0594.rst Resolved

Uh oh!

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

Comment thread pep-0594.rst Resolved

Uh oh!

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

Comment thread pep-0594.rst Resolved

Uh oh!

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

@bskinn @CAM-Gerlach
PEP 594:Apply suggested further edits from code review
8ba0ef6
Adds two missing commas and rewords the 'these packages 
are no longer slated for deprecation' language -- thanks, @CAM-Gerlach!

Co-authored-by: CAM Gerlach <[email protected]>
JelleZijlstra
JelleZijlstra reviewed Feb 16, 2022
View reviewed changes
Comment thread pep-0594.rst Resolved

Uh oh!

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

@hugovk

hugovk commented Feb 16, 2022

Copy link
Copy Markdown
Member

The Post-History at the top needs updating too:

Post-History: 21-May-2019

PEP 1:

  Post-History: <dates of postings to python-ideas and/or python-dev>

What is/are the new dates?

It was posted to Discourse on 4th Feb:

https://discuss.python.org/t/pep-594-take-2-removing-dead-batteries-from-the-standard-library/13508

(We probably need to include Discourse in PEP 1 too.)

All reactions

Sorry, something went wrong.

Uh oh!

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

@bskinn

bskinn commented Feb 16, 2022

Copy link
Copy Markdown
Contributor Author

@hugovk Would that be better fixed in a separate PR?

I had opened this one intending it not to change anything substantial.

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

@bskinn
Add further fixes
8dbc34e
- Harmonize header capitalization
- Add ``parser`` code formatting
- Add whitespacelines under "Update #" headers for consistency.
@CAM-Gerlach

CAM-Gerlach commented Feb 16, 2022

Copy link
Copy Markdown
Member

(We probably need to include Discourse in PEP 1 too.)

See #2266 ; a PR to implement what was discussed there is pending merge of #2302 , to avoid the chance of conflicts

I had opened this one intending it not to change anything substantial.

I'd consider that a pretty minor change, but I agree its out of scope here in a copyediting/proofreading PR. The discussion link should be updated as well; I can do that in a separate PR.

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

@bskinn

bskinn commented Feb 16, 2022

Copy link
Copy Markdown
Contributor Author

I'd consider that a pretty minor change, but I agree its out of scope here in a copyediting/proofreading PR.

Agreed that it's minor, but it seems like choosing the correct date to put there may take some discussion, which would probably be better sited with/near related discussion.

hugovk 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 mentioned this pull request Feb 16, 2022
PEP 594: Update Discussions-To link and date with posting of new thread #2335
Merged
@CAM-Gerlach

CAM-Gerlach commented Feb 16, 2022

Copy link
Copy Markdown
Member

PR opened as #2335 to update the Discourse link and post date

it seems like choosing the correct date to put there may take some discussion

I'm not sure how it is ambiguous, given its just the date that the new Discourse thread was posted (and not terribly critical regardless), but I do agree it is not really in scope here and would be better handled in a separate PR, which I've opened as above.

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

@bskinn

bskinn commented Feb 16, 2022

Copy link
Copy Markdown
Contributor Author

I'm not sure how it is ambiguous ...

Had just never noticed that the Post-History field could hold more than one date. 🙂

CAM-Gerlach reacted with thumbs up emoji hugovk reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

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

CAM-Gerlach
CAM-Gerlach approved these changes Feb 16, 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.

LGTM now, thanks @bskinn !

Sorry, something went wrong.

Uh oh!

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

bskinn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@CAM-Gerlach

CAM-Gerlach commented Feb 16, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

@brettcannon I assume you'll want to look this over and merge, since this is your PEP?

All reactions

Sorry, something went wrong.

Uh oh!

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

brettcannon
brettcannon approved these changes Feb 16, 2022
View reviewed changes
@brettcannon
brettcannon merged commit fc3eb21 into python:main Feb 16, 2022
@brettcannon

brettcannon commented Feb 16, 2022

Copy link
Copy Markdown
Member

@CAM-Gerlach 🤷 there's no semantic change here, so no need to wait on me specifically.

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.

@brettcannon

brettcannon commented Feb 16, 2022

Copy link
Copy Markdown
Member

@bskinn thanks for the PR!

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

@bskinn
bskinn deleted the patch-2 branch February 16, 2022 22:36
@erlend-aasland erlend-aasland mentioned this pull request May 13, 2022
pep8/greppable exception messages erlend-aasland/peps#1
Closed
@erlend-aasland erlend-aasland mentioned this pull request Jun 27, 2022
pep 687/mark as accepted erlend-aasland/peps#2
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@brettcannon brettcannon brettcannon approved these changes
@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes
@tiran tiran Awaiting requested review from tiran

Uh oh!

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

Assignees

No one assigned

Labels

CLA signed

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.

6 participants

@bskinn @hugovk @CAM-Gerlach @brettcannon @JelleZijlstra @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.