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.

Sign up for GitHub

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

Refactor for f strings. #984

Closed
wants to merge 1 commit into from
Closed

Refactor for f strings. #984

wants to merge 1 commit into from

Conversation

changeling
Copy link

@changeling changeling commented Jun 1, 2019 •
edited
Loading

Left the ``deprecated.py` alone for now, but it perhaps that deserves re-factoring for a very minor performance win?

Skipping some of relay/tests/test_connection_query.py for nested quoting laziness on my part.

@changeling changeling force-pushed the refactor-for-f-strings- branch 5 times, most recently from feb1c0c to 9811ddd Compare June 2, 2019 01:29
@ekampf
Copy link
Contributor

ekampf commented Jun 2, 2019

Your black pre-commit is failing

@ekampf
Copy link
Contributor

ekampf commented Jun 2, 2019

Also please rebase from next/master

ekampf
ekampf previously requested changes Jun 2, 2019
View reviewed changes
Copy link
Contributor

@ekampf ekampf left a comment

Choose a reason for hiding this comment

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

Per our discussion on slack, lets use this opportunity to consistently use either
f"{repr(value)} or f"{value!r}".
We decided on repr in Slack...

@changeling changeling force-pushed the refactor-for-f-strings- branch 2 times, most recently from ea6c9b2 to f7d1688 Compare June 2, 2019 18:34
@dvndrsn dvndrsn added the 3.0 Fix/Release version 3.0 label Jun 3, 2019
@dvndrsn
Copy link
Contributor

dvndrsn commented Jun 3, 2019

F-Strings are not supported in Python 2.7, so this PR would need to be merged in 3.0 when 2.7 and 3.4 support is dropped. Making sure this is tagged appropriately.

@changeling
Copy link
Author

F-Strings are not supported in Python 2.7, so this PR would need to be merged in 3.0 when 2.7 and 3.4 support is dropped. Making sure this is tagged appropriately.

Good note, and thanks. I'd not thought to do that, as it's targeted at next/master, which is 3 only, but I'll do that going forward.

@changeling
Refactor string .format() to f-string syntax. Minor corrections. Blac…
a432239 
…kened project.

Rebased to trigger ci.

Blackened project, due to Travis failures.

f-string formatting where applicable.
Minor corrections to language.

Corrected typo, missing parens.

Update test_structures.py to match updated phrasing.

Bowing to the wisdom of black.

As per black, adjusted line length in types/argument.py (line 67).

One  more kick in the pants by black.

More f-strings.

More blackening.
@changeling
Copy link
Author

Per our discussion on slack, lets use this opportunity to consistently use either
f"{repr(value)} or f"{value!r}".
We decided on repr in Slack...

@ekampf, "View changes" doesn't offer changes, so I'm unable to mark this change request as resolved.

@changeling changeling mentioned this pull request Jun 4, 2019
Closed
@jkimbo jkimbo mentioned this pull request Jun 4, 2019
Open
jkimbo
jkimbo approved these changes Jun 4, 2019
View reviewed changes
Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

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

👍 looks good. Minor suggestions but they don't block merge.

docs/relay/nodes.rst Show resolved Hide resolved
graphene/types/tests/test_structures.py Show resolved Hide resolved
graphene/types/tests/test_structures.py Show resolved Hide resolved
@changeling
Copy link
Author

This is gonna need to wait until we decide on 3.5 support.

@changeling
Copy link
Author

Lookd like we have concensus on 3.6+, so this is ready to go (or review more)!

@changeling
Copy link
Author

changeling commented Jun 19, 2019 •
edited
Loading

What's the status here? Requested changes made, though the system still shows Changes requested. I believe that's your #984 (review), @ekampf.

Can this be merged/reviewed so I can clear this from my end or address any new changes? @ProjectCheshire?

@jkimbo jkimbo requested a review from ekampf June 24, 2019 09:22
@changeling
Copy link
Author

Thoughts on reviewing/merging this, @ProjectCheshire, @ekampf, @dan98765 ?

@changeling changeling requested review from dvndrsn and phalt June 28, 2019 18:16
@stale
Copy link

stale bot commented Aug 27, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Aug 27, 2019
@stale stale bot closed this Sep 3, 2019
@jkimbo jkimbo reopened this Sep 27, 2019
@stale stale bot removed the wontfix label Sep 27, 2019
@stale
Copy link

stale bot commented Dec 26, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 26, 2019
@jkimbo jkimbo removed the wontfix label Dec 26, 2019
@KingDarBoja
Copy link

Any updates on this PR? Just curious as the author has no activity since July 2019 😨

@ekampf
Copy link
Contributor

ekampf commented Feb 6, 2020

@KingDarBoja its kind of stuck on conflicts it seem... @changeling ?

syrusakbary added a commit that referenced this pull request Mar 15, 2020
@syrusakbary
Updated all str.format(…) to f-strings
a22b09c 
This revamps the PR #984
@syrusakbary syrusakbary mentioned this pull request Mar 15, 2020
Merged
@syrusakbary
Copy link
Member

Apologizes for the delay actioning this PR.

This PR is taking the next steps to replace f-strings: #1158

syrusakbary added a commit that referenced this pull request Mar 15, 2020
@syrusakbary
Updated all str.format(…) to f-strings (#1158)
60a9609 
* Updated all str.format(…) to f-strings

This revamps the PR #984

* Pass black

* Fix flake8

* Updated objecttype

* Fix black version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkimbo jkimbo jkimbo approved these changes

@dan98765 dan98765 Awaiting requested review from dan98765

@ProjectCheshire ProjectCheshire Awaiting requested review from ProjectCheshire ProjectCheshire is a code owner

@ekampf ekampf Awaiting requested review from ekampf ekampf is a code owner

@dvndrsn dvndrsn Awaiting requested review from dvndrsn

@phalt phalt Awaiting requested review from phalt

Assignees
No one assigned
Labels
3.0 Fix/Release version 3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@changeling @ekampf @dvndrsn @KingDarBoja @syrusakbary @jkimbo