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

word-count: Add multiple apostrophes test case #2169

Merged
merged 9 commits into from
Feb 6, 2020

Conversation

c4llmeco4ch
Copy link
Contributor

This is a localized version of a change I discussed with the main problem-specs repo. They told me that, due to their lockdown, I should bring this to the language-specific repositories.

Effectively, the problem states that contractions ("can't", "won't", etc.) should treat apostrophes as part of the word, but rule 3 states:

Other than the apostrophe in a contraction all forms of punctuation are ignored
As such, I've added a very simple case testing that normal apostrophes are removed. The following code (while bad) should do a good job of showcasing a solution that passes all the live tests but fails the new one:

from string import punctuation
def count_words(sentence): 
    dict = {}
    punc = punctuation.replace("'",'')
    for val in punc:
        if val in sentence:
            sentence = sentence.replace(val, ' ')
    for word in sentence.split():
        if (w := word.lower())[0] == "'":
            w = w[1:]
        if w[-1] == "'":
            w = w[:-1]
        if w not in dict:
            dict[w] = 0
        dict[w] += 1
    return dict

Thanks!

@c4llmeco4ch c4llmeco4ch requested a review from a team as a code owner January 30, 2020 16:28
@cmccandless
Copy link
Contributor

Ref: exercism/problem-specifications#1628

@c4llmeco4ch c4llmeco4ch changed the title Added multiple apostrophes test case word-count: Add multiple apostrophes test case Jan 30, 2020
@yawpitch
Copy link
Contributor

Slight divergence, but what the heck should happen with a plural possessive? For instance, what should the word count be for the phrase "lawyers accept both lawyer's fees and lawyers' fees?"?

Copy link
Contributor

@yawpitch yawpitch left a comment

Choose a reason for hiding this comment

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

I'm okay with this test as is, though we are getting to the point where only a regex-based solution will work.

Copy link
Contributor

@cmccandless cmccandless left a comment

Choose a reason for hiding this comment

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

Please check out the docs on adding track specific tests and regenerating the test suite. CI will not pass if word_count_test.py is edited directly.

@c4llmeco4ch
Copy link
Contributor Author

Sounds good, I'll get on that later today. I appreciate the help from everyone

@c4llmeco4ch
Copy link
Contributor Author

Really sorry for possibly blowing up everyone's feed. After generating tests from the automated script, I'm still getting failures from Travis. Do I need to revert the test script back to the original passing version then push without the test added? Thanks and sorry again for the mess.

@c4llmeco4ch
Copy link
Contributor Author

So I double checked and the reason I was pushing two versions of the test was due to my local version of problem-specs contained the test from my discussion with the main repo. I updated that but I'm still having the same failures from Travis.

@yawpitch
Copy link
Contributor

yawpitch commented Feb 2, 2020

Did you actually set the branch in your local copy of problem-specifications to master, rather than just remove the test in your working branch? The version number shouldn't be bumped in the generated tests, since the canonical-data.json it's pulling from hasn't (in master) been versioned.

@c4llmeco4ch
Copy link
Contributor Author

c4llmeco4ch commented Feb 3, 2020

I'll take a look Monday. That could be the issue.

@c4llmeco4ch
Copy link
Contributor Author

Good call @yawpitch

@cmccandless cmccandless merged commit b898807 into exercism:master Feb 6, 2020
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.

3 participants