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

Merged
merged 15 commits into from
Dec 29, 2024

Conversation

sanks011
Copy link
Contributor

@sanks011 sanks011 commented Dec 16, 2024

Describe your change:

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 #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
Contributor 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
strings/join.py Outdated
for i, element in enumerate(separated):
result += element
if i < len(separated) - 1: # Add separator only between elements
result += separator

Copy link
Contributor 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?

strings/join.py Outdated

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
Contributor 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
@MaximSmolskiy
Copy link
Member

Reversed split changes in favor #12423

Copy link
Member

@MaximSmolskiy MaximSmolskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this PR to several - one PR for one issue (especially if the issues are not related as in this case)

And this PR has failed doctests

>>> join("#", "a")  # Single string instead of list
Expected:
    Traceback (most recent call last):
        ...
    Exception: join() accepts only strings
Got:
    'a'

@algorithms-keeper algorithms-keeper bot added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels Dec 29, 2024
@algorithms-keeper algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed and removed awaiting changes A maintainer has requested changes to this PR labels Dec 29, 2024
@MaximSmolskiy
Copy link
Member

Reversed join changes in favor of #12434

@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Dec 29, 2024
Copy link
Member

@MaximSmolskiy MaximSmolskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanks011 Thanks for fix for #12346

@algorithms-keeper algorithms-keeper bot removed the awaiting reviews This PR is ready to be reviewed label Dec 29, 2024
@MaximSmolskiy MaximSmolskiy merged commit d9092d8 into TheAlgorithms:master Dec 29, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning sign after completing installation of all the requirements
4 participants