-
Notifications
You must be signed in to change notification settings - Fork 371
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
Improvement: error_msg changed to f-strings #874
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for going through the code and changing all the error messages! Looks good to me besides one issue at the bottom.
One extra thing that would be nice is to shorten the line length of these messages to conform to PEP-8. I think you can automate that by running --experimental-string-processing
with black.
Co-authored-by: Colin McAllister <colinmca242@gmail.com>
Codecov Report
@@ Coverage Diff @@
## develop #874 +/- ##
========================================
Coverage 77.96% 77.96%
========================================
Files 64 64
Lines 3608 3608
========================================
Hits 2813 2813
Misses 795 795
|
@colin-pm f-string for other strings like |
@xunzhou The more f-strings, the better imo The pylint errors could be fixed by running black with the |
085b858
to
9dbfa98
Compare
The Since your PR was opened before this change, I have:
You will need to pull down the latest
Because this PR has conflicts, I have not done the rebase myself, but if you need a hand, let me know. |
As @colin-pm suggested in #873 (comment),
updating
error_msg
to f-string