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

Use f-strings #419

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Use f-strings #419

wants to merge 14 commits into from

Conversation

cmaureir
Copy link

After #403 is merged, there will be no reason to use
the old string styling from Python2, and since
f-strings are available since Python 3.6 it could
be adopted as the default style.

After voila-dashboards#403 is merged, there will be no reason to use
the old string styling from Python2, and since
f-strings are available since Python 3.6 it could
be adopted as the default style.
@SylvainCorlay
Copy link
Member

Thanks @cmaureir. This looks good to me.

We may be stuck with Python 3.5 for a few more months (december).

@cmaureir
Copy link
Author

Np @SylvainCorlay I can do the refactoring once 3.5 is out of the picture

@jtpio
Copy link
Member

jtpio commented Oct 13, 2019

Thanks @cmaureir for taking the time to do this.

I'm also waiting for the day when we can drop everything below 3.5 :)

@maartenbreddels
Copy link
Member

Happy to merge this in the future with the f-strings on lines that do not use the logger. However, using log.<topic>(formatstring, *args) does not do any formatting if the log msg does not go anywhere while the f-strings will always will be evaluated, leading to possible performance issues. We should go over this PR and made sure we don't merge those lines.

@jtpio
Copy link
Member

jtpio commented Jul 14, 2020

#403 has been merged, and Python 3.5 shouldn't be a requirement anymore.

So we should now be able to switch to using f-strings (omitting the changes to the logger) 👍

@cmaureir
Copy link
Author

#403 has been merged, and Python 3.5 shouldn't be a requirement anymore.

So we should now be able to switch to using f-strings (omitting the changes to the logger) +1

not sure if I touched something related to the logger while solving all the conflicts, but let me know in case I did it 👯

setup.py Outdated Show resolved Hide resolved
voila/app.py Outdated Show resolved Hide resolved
voila/app.py Outdated Show resolved Hide resolved
voila/app.py Outdated Show resolved Hide resolved
voila/app.py Outdated Show resolved Hide resolved
voila/app.py Outdated Show resolved Hide resolved
voila/treehandler.py Outdated Show resolved Hide resolved
Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
@cmaureir
Copy link
Author

thanks @jtpio, let's see now.

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
tests/app/notebooks_test.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
voila/execute.py Outdated Show resolved Hide resolved
Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
tests/app/cwd_subdir_test.py Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented Jul 14, 2020

Looking good :)

image

@jtpio
Copy link
Member

jtpio commented Jan 31, 2024

@trungleduc mind providing more context for closing this PR?

Probably because this was handled in a separate PR, for example after switching to black? (not sure we use ruff here yet)

@trungleduc
Copy link
Member

trungleduc commented Jan 31, 2024

Thanks @jtpio for the double-check! I closed the old PRs thinking it was not worth the rebasing effort. Actually, the conflicts in this PR are quite easy to fix, reopening it.

@trungleduc trungleduc reopened this Jan 31, 2024
@jtpio
Copy link
Member

jtpio commented Jan 31, 2024

OK nice thanks for checking @trungleduc 👍

Yeah I remember this PR was created during the Voila Sprint at some previous PyData Berlin conference. Maybe we can help Cristián wrap it up so it can be merged :)

@cmaureir
Copy link
Author

oh, super old PR sorry for not reacting before 😆 I tried to solve all the merge conflicts, but I'm certain some styling checks might fail. Let's see what the CI needs to tell us 👯

@trungleduc
Copy link
Member

Thanks @cmaureir!

@jtpio
Copy link
Member

jtpio commented Jan 31, 2024

super old PR sorry for not reacting before 😆

haha thanks @cmaureir for your patience! 🙏

@cmaureir
Copy link
Author

cmaureir commented Jan 31, 2024

I found new ones!
I think after these, we are good to go.

Please notice that "... %r ..." % var cases, were replaced to f"...{var!r}...",
but also there were some cases where %i was used, which doesn't have an equivalent to use in f-strings, so I could use {var:.0f} in order to bypass it, but it adds a float-conversion, but I decided to left them like f"{var}..." only, because the variables was most certainly a string already,.

@maartenbreddels
Copy link
Member

However, using log.<topic>(formatstring, *args) does not do any formatting if the log msg does not go anywhere while the f-strings will always will be evaluated, leading to possible performance issues. We should go over this PR and made sure we don't merge those lines.

Note that this still applies. All lines calling the logger should be reverted if we do not want worse performance

@cmaureir
Copy link
Author

cmaureir commented Feb 4, 2024

oh, that's unfortunate, will try to amend the change then

@cmaureir
Copy link
Author

cmaureir commented Feb 4, 2024

I hope everything is in order now @maartenbreddels

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

Successfully merging this pull request may close these issues.

None yet

5 participants