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

MNT: Use f-strings where possible #369

Closed
wants to merge 1 commit into from
Closed

MNT: Use f-strings where possible #369

wants to merge 1 commit into from

Conversation

DimitriPapadopoulos
Copy link
Contributor

Use .format() where f-strings would be unreadable.

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as draft September 28, 2023 15:12
@pep8speaks
Copy link

pep8speaks commented Sep 28, 2023

Hello @DimitriPapadopoulos! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-09-28 15:17:19 UTC

Use .format() where f-strings would be unreadable.
@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review September 28, 2023 15:17
@effigies
Copy link
Member

I'd rather not merge cosmetic changes in before we merge next into master.

Also, % is not deprecated functionality, so let's not "upgrade" to .format() just because it's newer.

@effigies effigies closed this Sep 28, 2023
@DimitriPapadopoulos DimitriPapadopoulos deleted the f-strings branch September 28, 2023 19:10
@DimitriPapadopoulos
Copy link
Contributor Author

The idea is that f-strings are faster, not newer.

@effigies
Copy link
Member

To be clear, I'm fine upgrading to f-strings where it works and is clean. But when we're using % instead of .format(), it's usually because it's a delayed interpolation and we decided % was the cleaner of the two options.

@DimitriPapadopoulos
Copy link
Contributor Author

I understand. In any case, isn't it better to wait for next to be merged into master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants