-
-
Notifications
You must be signed in to change notification settings - Fork 46k
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
fixes join.py and split.py action and requirements error #12438
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
strings/join.py
Outdated
if not all(isinstance(word_or_phrase, str) for word_or_phrase in separated): | ||
raise Exception("join() accepts only strings") | ||
|
||
joined = separator.join(separated) |
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.
Question about this join though. Isn't the point of this repository to show possible implementations of python functions? Because if that's the case, I was wondering why the join function now internally uses str.join?
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.
okay I understood! Now I'll try to implement something which manually constructs the resulting string without relying on Python's built-in str.join()
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.
seems fine but why the join function now internally uses str.join? as what @Kaaserne said
for more information, see https://pre-commit.ci
for i, element in enumerate(separated): | ||
result += element | ||
if i < len(separated) - 1: # Add separator only between elements | ||
result += separator | ||
|
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.
@Kaaserne can you please review it?
|
||
Examples: | ||
|
||
>>> join("", ["a", "b", "c", "d"]) |
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.
What's the reason these got removed? I've never used doctest
before but I think this has something to do with testing. Maybe you can also add:
"""
>>> join(',', ['', '', ''])
',,'
"""
strings/join.py
Outdated
""" | ||
if not all(isinstance(word_or_phrase, str) for word_or_phrase in separated): |
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 this if statement, with the removal of all
, can be placed inside the for-loop. That way we only iterate over the sequence once, instead of twice.
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.
Thank you for your advice. sure lemme check
for more information, see https://pre-commit.ci
Describe your change:
Fixes #12408
Fixes #12403
Fixes #12346
Checklist:
For #12408
Previously it was
Now
new version utilizes Python's built-in separator.join(separated) to join the strings. This method is more efficient and handles edge cases (like empty strings) properly without needing manual intervention and all() function to check if all elements in the separated list are strings before proceeding. This avoids checking each element individually during the loop.
For #12403
Previously it was
Now
The updated code includes a check at the end (if string and string[-1] == separator) to ensure that if the string ends with a separator, an empty string '' is added to the list. This ensures the behavior matches Python's built-in split method.
For #12346