Skip to content

/ Python

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

fix: space count in strings/word_occurrence.py #1896

Merged
merged 5 commits into from Apr 21, 2020
Merged

fix: space count in strings/word_occurrence.py #1896

merged 5 commits into from Apr 21, 2020

Conversation

@darkmatter18
Copy link
Contributor

darkmatter18 commented Apr 20, 2020 •
edited

Describe your change:

Fixed the space count in the strings/word_occurrence.py

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • 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 have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #1895

Fixes: #1895
@darkmatter18
fix: space count in strings/word_occurrence.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
602f94b
@cclauss
cclauss reviewed Apr 20, 2020
View changes
strings/word_occurrence.py Outdated Show resolved Hide resolved
@darkmatter18 @cclauss
Update strings/word_occurrence.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
891290e 
Co-Authored-By: Christian Clauss <cclauss@me.com>
@cclauss
cclauss requested changes Apr 21, 2020
View changes
Copy link
Member

cclauss left a comment

Sorry but I do not believe this PR is doing anything useful. The proposed code is currently not popping out the space character but is instead popping out the empty string. I do not see how this algorithm is putting space or the empty string into occurrences. I do not believe that len(occurrences) will change before and after the pop. We need a doctest that proves this change is needed.
@darkmatter18
Copy link
Contributor Author

darkmatter18 commented Apr 21, 2020

Hey @cclauss , I found that after spliting the space characters create blank strings. It wound be hard to found and remove the spaces from the whole string. So let them store them as blank character count and then pop the count out. I added a screenshot of the code running on my machine with Before Popping and After Popping. You can find the Blank before popping, but not after. Thanks

Annotation 2020-04-21 110755
@darkmatter18 darkmatter18 requested a review from cclauss Apr 21, 2020
@cclauss
cclauss reviewed Apr 21, 2020
View changes
strings/word_occurrence.py Outdated Show resolved Hide resolved
@darkmatter18 @cclauss
Update strings/word_occurrence.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
2cf062e 
Co-Authored-By: Christian Clauss <cclauss@me.com>
@darkmatter18 darkmatter18 requested a review from cclauss Apr 21, 2020
@cclauss
cclauss reviewed Apr 21, 2020
View changes
strings/word_occurrence.py Outdated Show resolved Hide resolved
@cclauss
cclauss reviewed Apr 21, 2020
View changes
strings/word_occurrence.py Show resolved Hide resolved
@cclauss
Copy link
Member

cclauss commented Apr 21, 2020

Also, please fix the spelling of occurrence (two c's and two r's) in the function name.
darkmatter18 and others added 2 commits Apr 21, 2020
@darkmatter18 @cclauss
Update strings/word_occurrence.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
7ceac2e 
Co-Authored-By: Christian Clauss <cclauss@me.com>
@darkmatter18
Update word_occurrence.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
39fe3b9 
Seems like, there is no need o `occurrence.pop('', None)`
@darkmatter18
Copy link
Contributor Author

darkmatter18 commented Apr 21, 2020

@cclauss Seems like, there is no need o occurrence.pop('', None)
@darkmatter18 darkmatter18 requested a review from cclauss Apr 21, 2020
@cclauss
cclauss approved these changes Apr 21, 2020
View changes
Copy link
Member

cclauss left a comment

Awesome detective work here to find and fix a hard to spot bug! Thanks for your contribution.
@cclauss cclauss merged commit 098be35 into TheAlgorithms:master Apr 21, 2020
3 checks passed
3 checks passed
codespell
Details
LGTM analysis: Python No new or fixed alerts
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cclauss cclauss

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Need to modify the word occurrence algorithm
2 participants
@darkmatter18 @cclauss
You can’t perform that action at this time.