Skip to content

word-count: Add multiple apostrophes test case - #2169

Merged
cmccandless merged 9 commits into
exercism:masterfrom
c4llmeco4ch:master
Feb 6, 2020
Merged

word-count: Add multiple apostrophes test case#2169
cmccandless merged 9 commits into
exercism:masterfrom
c4llmeco4ch:master

Conversation

@c4llmeco4ch

@c4llmeco4ch c4llmeco4ch commented Jan 30, 2020

Copy link
Copy Markdown
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

cmccandless commented Jan 30, 2020

Copy link
Copy Markdown
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

yawpitch commented Jan 31, 2020

Copy link
Copy Markdown
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?"?

yawpitch
yawpitch approved these changes Jan 31, 2020
View reviewed changes

@yawpitch yawpitch left a comment

Copy link
Copy Markdown
Contributor

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.

cmccandless
cmccandless suggested changes Jan 31, 2020
View reviewed changes

@cmccandless cmccandless left a comment

Copy link
Copy Markdown
Contributor

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

c4llmeco4ch commented Jan 31, 2020

Copy link
Copy Markdown
Contributor Author

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

@c4llmeco4ch

c4llmeco4ch commented Feb 1, 2020

Copy link
Copy Markdown
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

c4llmeco4ch commented Feb 1, 2020

Copy link
Copy Markdown
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

yawpitch commented Feb 2, 2020

Copy link
Copy Markdown
Contributor

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

c4llmeco4ch commented Feb 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@c4llmeco4ch

c4llmeco4ch commented Feb 4, 2020

Copy link
Copy Markdown
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

Reviewers

2 more reviewers
@yawpitch yawpitch yawpitch approved these changes
@cmccandless cmccandless cmccandless approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@c4llmeco4ch @cmccandless @yawpitch