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

Make rhymes return rhymes for all pronunciations #46

Merged
merged 7 commits into from Sep 5, 2018

Conversation

@jdbean
Copy link
Contributor

@jdbean jdbean commented Aug 31, 2018

Rhymes now returns a single, flat, sorted list that contains the words rhyming with all pronunciations of the given word.

Closes #45

jdbean added 2 commits Aug 31, 2018
Rhymes now returns a single, flat, sorted list that contains the words rhyming with all pronunciations of the given word.
pronouncing/__init__.py Outdated Show resolved Hide resolved
Loading
Correcting erroneous  indent
element), []) if w != word])
combined_rhymes = list(chain.from_iterable(combined_rhymes))
combined_rhymes.sort()
unique_combined_rhymes = set(combined_rhymes)
Copy link
Owner

@aparrish aparrish Sep 1, 2018

Choose a reason for hiding this comment

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

iirc, I'm not sure if using set/list to remove duplicates will produce the desired results across all the versions of python that we're targeting—probably best to sort the list post-dedup.

Loading

Copy link
Contributor Author

@jdbean jdbean Sep 2, 2018

Choose a reason for hiding this comment

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

Thanks! I switched the order of operations and I think it should be working correctly now.

Loading

@aparrish
Copy link
Owner

@aparrish aparrish commented Sep 1, 2018

also this needs an update to the test_rhymes test (which is why the CI is currently failing). can you update that test and push to this same PR? (If you don't have time or prefer not to, that's fine—I can do it, but it might take a while for me to get around to it.)

Loading

Accounting sorting unique_combine_rhymes after dedup in order to account for behavior of set
@jdbean
Copy link
Contributor Author

@jdbean jdbean commented Sep 2, 2018

This newest push seems to have resolved the CI failures. I may be able to look into updating the tests to avoid future regressions if you're not comfortable merging as is.

Loading

@aparrish
Copy link
Owner

@aparrish aparrish commented Sep 3, 2018

actually if you could add a test to avoid regressions on this that would be great!

Loading

@jdbean
Copy link
Contributor Author

@jdbean jdbean commented Sep 3, 2018

I think that should do it! 😄 Please let me know if you have any feedback.

Loading

tests/test_pronouncing.py Outdated Show resolved Hide resolved
Loading
Breaking up rhymes test assertions into independent methods
@jdbean
Copy link
Contributor Author

@jdbean jdbean commented Sep 4, 2018

@hugovk Thanks for the feedback. I agree that the rhymes tests are better off being split into their own methods. I've pushed a new commit which splits them out as such.

Loading

@aparrish aparrish merged commit 9a1c5ff into aparrish:master Sep 5, 2018
2 checks passed
Loading
@aparrish
Copy link
Owner

@aparrish aparrish commented Sep 5, 2018

looks good, thanks! will include in next release.

Loading

@jdbean
Copy link
Contributor Author

@jdbean jdbean commented Sep 5, 2018

Most welcome. Glad I could help!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants