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

Jump to bottom

bpo-38351: Modernize email example from %-formatting to f-string #17162

Merged
merged 3 commits into from Nov 15, 2019
Merged

bpo-38351: Modernize email example from %-formatting to f-string #17162

merged 3 commits into from Nov 15, 2019

Conversation

dorosch
Copy link
Contributor

@dorosch dorosch commented Nov 15, 2019 •
edited by bedevere-bot

@dorosch dorosch requested a review from a team as a code owner Nov 15, 2019
@dorosch dorosch force-pushed the modernize_email_examples branch from 301b3b0 to 9583f64 Compare Nov 15, 2019
@brandtbucher
Copy link
Member

brandtbucher commented Nov 15, 2019

Thanks for the PR @dorosch!

@brandtbucher brandtbucher added the docs Documentation in the Doc dir label Nov 15, 2019
brandtbucher
brandtbucher requested changes Nov 15, 2019
View changes
Copy link
Member

@brandtbucher brandtbucher left a comment

I agree these should be updated, but f-strings are actually templated string literals (str.format is a third, different way of formatting strings).

You're almost there though... see my suggestions below:

Doc/includes/email-dir.py Outdated Show resolved Hide resolved
Doc/includes/email-simple.py Outdated Show resolved Hide resolved
Doc/includes/email-unpack.py Outdated Show resolved Hide resolved
@dorosch
Copy link
Contributor Author

dorosch commented Nov 15, 2019 •
edited

I agree these should be updated, but f-strings are actually templated string literals (str.format is a third, different way of formatting strings).

You're almost there though... see my suggestions below:

Thanks for your feedback. I used str.format as in other examples of documentation used str.format

brandtbucher
brandtbucher requested changes Nov 15, 2019
View changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Great, just a couple more comments!

Doc/includes/email-dir.py Outdated Show resolved Hide resolved
Doc/includes/email-dir.py Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Documentation/2019-11-15-09-22-28.bpo-38351.xwhlse.rst Outdated Show resolved Hide resolved
@brandtbucher
Copy link
Member

brandtbucher commented Nov 15, 2019

The CI failure is unrelated. Pushing to this branch should trigger a new build.

aeros
aeros requested changes Nov 15, 2019
View changes
Copy link
Member

@aeros aeros left a comment

Thanks for the PR @dorosch and welcome! Other than a few minor details, the PR looks most of the way there after @brandtbucher's suggestions.

Doc/includes/email-unpack.py Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Documentation/2019-11-15-09-22-28.bpo-38351.xwhlse.rst Outdated Show resolved Hide resolved
Doc/includes/email-dir.py Outdated Show resolved Hide resolved
@aeros
Copy link
Member

aeros commented Nov 15, 2019 •
edited

It looks like the travis build failure was not related to the PR, committing the latest suggestions will restart the CI (or closing and reopening the PR).

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

@brandtbucher
Copy link
Member

brandtbucher commented Nov 15, 2019

Looks like we crossed posts @aeros. 🙂

@aeros
Copy link
Member

aeros commented Nov 15, 2019

Looks like we crossed posts @aeros. slightly_smiling_face

Haha, oops. 🙂

@brandtbucher
Copy link
Member

brandtbucher commented Nov 15, 2019

Good to see our comments are almost entirely identical. Haha.

@aeros
Copy link
Member

aeros commented Nov 15, 2019

These changes can be backported to the 3.8 and 3.7 docs as well.

@dorosch
Copy link
Contributor Author

dorosch commented Nov 15, 2019

These changes can be backported to the 3.8 and 3.7 docs as well.

Could you tell me how I can do this?

@brandtbucher
Copy link
Member

brandtbucher commented Nov 15, 2019

That's a note for the core reviewer. Nothing required on your part @dorosch!

brandtbucher
brandtbucher approved these changes Nov 15, 2019
View changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Awesome.

aeros
aeros approved these changes Nov 15, 2019
View changes
Copy link
Member

@aeros aeros left a comment

LGTM, thanks for applying the suggested changes @dorosch.

@aeros
Copy link
Member

aeros commented Nov 15, 2019 •
edited

@dorosch

Could you tell me how I can do this?

@brandtbucher

That's a note for the core reviewer. Nothing required on your part @dorosch!

Correct, but more specifically: when the needs backport to <x> label is applied, the bot @miss-islington will attempt to automatically open a backport PR to the branch for version <x>, after the original PR is merged. Sometimes when there are merge conflicts, it is required to use the cherry-picker tool (https://pypi.org/project/cherry-picker/) to resolve them and manually open the backport PR(s), but most of the time it can be done automatically (especially for minor changes).

@dorosch
Copy link
Contributor Author

dorosch commented Nov 15, 2019

@dorosch

Could you tell me how I can do this?

@brandtbucher

That's a note for the core reviewer. Nothing required on your part @dorosch!

Correct, but more specifically: when the needs backport to <x> label is applied, the bot @miss-islington will attempt to automatically open a backport PR to the branch for version <x>, after the original PR is merged. Sometimes when there are merge conflicts, it is required to use the cherry-picker tool (https://pypi.org/project/cherry-picker/) to resolve them and manually open the backport PR(s), but most of the time it can be done automatically (especially for minor changes).

Thanks for your explanation!

taleinat
taleinat approved these changes Nov 15, 2019
View changes
Copy link
Contributor

@taleinat taleinat left a comment

LGTM

@taleinat taleinat merged commit e8acc86 into python:master Nov 15, 2019
@miss-islington
Copy link
Contributor

miss-islington commented Nov 15, 2019

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 15, 2019
@miss-islington
bpo-38351: Modernize email examples from %-formatting to f-strings (p…
3ba0fdd 
…ythonGH-17162)

(cherry picked from commit e8acc86)

Co-authored-by: Andrey Doroschenko <dorosch.github.io@yandex.ru>
@bedevere-bot
Copy link

bedevere-bot commented Nov 15, 2019

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

@bedevere-bot
Copy link

bedevere-bot commented Nov 15, 2019

GH-17168 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 15, 2019
@miss-islington
bpo-38351: Modernize email examples from %-formatting to f-strings (p…
037f8b4 
…ythonGH-17162)

(cherry picked from commit e8acc86)

Co-authored-by: Andrey Doroschenko <dorosch.github.io@yandex.ru>
miss-islington added a commit that referenced this pull request Nov 15, 2019
@miss-islington
bpo-38351: Modernize email examples from %-formatting to f-strings (G…
dae27cc 
…H-17162)

(cherry picked from commit e8acc86)

Co-authored-by: Andrey Doroschenko <dorosch.github.io@yandex.ru>
miss-islington added a commit that referenced this pull request Nov 15, 2019
@miss-islington
bpo-38351: Modernize email examples from %-formatting to f-strings (G…
fe67a53 
…H-17162)

(cherry picked from commit e8acc86)

Co-authored-by: Andrey Doroschenko <dorosch.github.io@yandex.ru>
@brandtbucher
Copy link
Member

brandtbucher commented Nov 15, 2019

Congrats on your first CPython contribution @dorosch! 🍾

Looking forward to seeing more from you in the future.

@dorosch
Copy link
Contributor Author

dorosch commented Nov 15, 2019

Congrats on your first CPython contribution @dorosch! champagne

Looking forward to seeing more from you in the future.

@brandtbucher Thank you very much for your help

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
bpo-38351: Modernize email examples from %-formatting to f-strings (p…
8073165 
…ythonGH-17162)
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@shihai1991
bpo-38351: Modernize email examples from %-formatting to f-strings (p…
f847ee9 
…ythonGH-17162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericvsmith ericvsmith

@taleinat taleinat

@brandtbucher brandtbucher

@aeros aeros

@python/email-team email-team

Assignees
No one assigned
Labels
docs Documentation in the Doc dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@dorosch @brandtbucher @aeros @miss-islington @bedevere-bot @ericvsmith @taleinat @the-knights-who-say-ni