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

fixes join.py and split.py action and requirements error #12438

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

sanks011
Copy link

@sanks011 sanks011 commented Dec 16, 2024

Describe your change:

Fixes #12408
Fixes #12403
Fixes #12346

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Add or change doctests? -- Note: Please avoid changing both code and tests in a single pull request.
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the description above includes the issue number(s) with a closing keyword: "Fixes #ISSUE-NUMBER".

For #12408
Previously it was
image
image

Now
image
image

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
image
Now
image

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
image

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Dec 16, 2024
@algorithms-keeper algorithms-keeper bot added the awaiting reviews This PR is ready to be reviewed label Dec 16, 2024
@sanks011 sanks011 changed the title fixes join.py action fixes join.py and split.py action Dec 16, 2024
@sanks011 sanks011 changed the title fixes join.py and split.py action fixes join.py and split.py action and requirements error Dec 16, 2024
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)
Copy link

@Kaaserne Kaaserne Dec 16, 2024

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?

Copy link
Author

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()

Copy link
Contributor

@imSanko imSanko left a 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

@algorithms-keeper algorithms-keeper bot removed tests are failing Do not merge until tests pass labels Dec 18, 2024
for i, element in enumerate(separated):
result += element
if i < len(separated) - 1: # Add separator only between elements
result += separator

Copy link
Author

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"])
Copy link

@Kaaserne Kaaserne Dec 18, 2024

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):
Copy link

@Kaaserne Kaaserne Dec 18, 2024

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.

Copy link
Author

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

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting reviews This PR is ready to be reviewed tests are failing Do not merge until tests pass
Projects
None yet
3 participants