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
Add utility function to strip '.py' extensions #3658
Conversation
The previous use of str.strip(".py") is a bug waiting to happen, because it strips from both sides of the string (instead of just the end), and more importantly, treats ".py" as a set of characters rather than a substring to look for: $ python3 >>> "ppp.yyy".strip(".py") '' >>> "no_dot_py".strip(".py") 'no_dot_' >>> ".py_prefix".strip(".py") '_prefix'
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
I think your concerns are valid! Just as an alternate suggestion, though, how do you feel about using str.removesuffix
instead? It's also mentioned in the docs for str.rstrip
.
That way there's one fewer utility function we have to maintain in our codebase.
EDIT: I missed the "New in version 3.9". Oops! We can't use the feature that I suggested, since we're on Python 3.7 atm.
Until we upgrade to Python 3.9, which has this string method as a builtin, we can use the code from PEP 616.
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.
LGTM! Thank you for this change. :)
Checklist
GitHub
PR contents
Description
The previous use of
str.strip(".py")
is a bug waiting to happen,because it strips from both sides of the string (instead of just
the end), and more importantly, treats
".py"
as a set ofcharacters rather than a substring to look for (see the docs):
All uses of
.strip(".py")
are either inutils/sys.py
itself or import this module, so it seemed like the natural place to add this utility function.Linked issues