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

replace notify format usage with f-strings instead #1409

Closed
ctb opened this issue Mar 27, 2021 · 5 comments · Fixed by #1723
Closed

replace notify format usage with f-strings instead #1409

ctb opened this issue Mar 27, 2021 · 5 comments · Fixed by #1723
Labels
good first issue python Pull requests that update Python code repeatable quest Issues with many, many subtasks; good for people new to codebase.

Comments

@ctb
Copy link
Contributor

ctb commented Mar 27, 2021

throughout the Python code base, it would be nice to replace calls like,

notify("file {} is bad", filename)

(which uses string.format underneath)

with f-strings, e.g.

notify(f"file {filename} is bad")

One motivation for this is that error messages with f-strings that contain strings or filenames with {} in them break the current notify function. The other is that f-strings are really nice and simple!

Questions/thoughts -

  • one easy way to find places to fix this up is to (temporarily) break the current notify function so that if it passed any arguments, it barfs with an assert 0.
  • should we create a new function, notify_verbatim or something, or just work to replace notify with something that no longer uses string.format?

examples

see #1406 for an example of doing this in sourmash_args.py.

also see #1342 for a different set of changes to notify.

more info

see https://realpython.com/python-f-strings/ for more on f-strings.

@ctb ctb added good first issue python Pull requests that update Python code repeatable quest Issues with many, many subtasks; good for people new to codebase. labels Mar 27, 2021
@ctb ctb changed the title replace notify with f-string replace notify with f-strings Mar 27, 2021
@itsabhianant
Copy link
Contributor

Hi @ctb,

I have read your issue and I came to the conclusion that you want to enhance the project by replacing the string.formats with the f strings. I think I can work on this project and help you by enhancing the code base as per your wish. Please assign these issue to me and also give me your mail-id so that I can contact you in case I need help.

Best,
Abhishek

@ctb
Copy link
Contributor Author

ctb commented Mar 27, 2021

hi @itsabhianant just ask for help here on this issue, or in a newly opened pull request!

please start small, with just one change to one place. note that notify uses string.format underneath, but we can ignore that for now and just replace calls to notify(x, value) with notify(f"{value}").

resources for new sourmash developers

developer docs

the github flow process

@itsabhianant
Copy link
Contributor

itsabhianant commented Mar 27, 2021

Ok, can you tell me which specific directories do you want the changes to be made to? Also, can you assign this issue to me?
Like you want me to replace the .format in every file or only in specific ones.

@ctb ctb changed the title replace notify with f-strings replace notify format usage with f-strings instead Mar 28, 2021
@ctb
Copy link
Contributor Author

ctb commented Mar 28, 2021

hi @itsabhianant I created #1418 for you as a specific issue. I can't assign it to you without making you part of this organization, which I'm not ready to do yet, but you are welcome to claim it. please mention both that issue and this issue in the pull request you create. thanks!

@keyabarve
Copy link
Contributor

keyabarve commented Aug 11, 2021

@ctb I'd like to work on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue python Pull requests that update Python code repeatable quest Issues with many, many subtasks; good for people new to codebase.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants