Skip to content

pangram tests updated to 1.3 #1039

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

Merged
merged 7 commits into from
Jan 9, 2018
Merged

pangram tests updated to 1.3 #1039

merged 7 commits into from
Jan 9, 2018

Conversation

luoken
Copy link
Contributor

@luoken luoken commented Oct 25, 2017

updated to version 1.3 and moved one test to the bottom

Resolves #1018

@ilya-khadykin
Copy link
Contributor

@luoken could you please rename your PR to something more meaningful and add a link to the issue in the description

@cmccandless
Copy link
Contributor

@luoken Thanks for you pull request! As #1018 is specifically for the pangram exercise, please limit your changes in the PR to only those files. If you would like to update pig-latin as well, please create a separate PR referencing #1019.

@luoken luoken changed the title Issue1018 pangram tests updated to 1.3 Oct 25, 2017
@luoken
Copy link
Contributor Author

luoken commented Oct 25, 2017

link to issue : #1018

@N-Parsons
Copy link
Contributor

@luoken, you still have changes committed to multiple exercises within this PR. Since the changes for pig-latin are already being implemented in #1034, please limit this PR to pangram.

changed to default tests for pig latin

def test_another_missing_character_x(self):
self.assertIs(
is_pangram('the quick brown fish jumps over the lazy dog'),
False)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that this additional test really adds any coverage?

@@ -42,14 +43,20 @@ def test_missing_letters_replaced_by_numbers(self):

def test_pangram_with_mixedcase_and_punctuation(self):
self.assertIs(
is_pangram('"Five quacking Zephyrs jolt my wax bed."'),
is_pangram('\"Five quacking Zephyrs jolt my wax bed.\"'),
Copy link
Contributor

Choose a reason for hiding this comment

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

The \" construct is for escaping the quote in the json - I don't think that they are needed here.

@@ -2,13 +2,16 @@

from pangram import is_pangram


# test cases adapted from `x-common//canonical-data.json` @ version: 1.0.0
# test cases adapted from `x-common//canonical-data.json` @ version: 1.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This should reference problem-specifications, ie:

# Tests adapted from `problem-specifications//canonical-data.json` @ v1.3.0

@N-Parsons
Copy link
Contributor

@luoken, this PR is currently failing Travis-CI check due to flake8 errors (see results). If you're still interested in finishing this PR, these errors need to be fixed, along with a few minor changes that I've pointed out with comments.

@stale
Copy link

stale bot commented Dec 6, 2017

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the abandoned label Dec 6, 2017
@stale stale bot removed the abandoned label Dec 8, 2017
@cmccandless
Copy link
Contributor

@N-Parsons As this PR abandoned, but nearly finished, I went ahead and made the requested change. If there is nothing else, go ahead and merge.

@stale
Copy link

stale bot commented Dec 29, 2017

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the abandoned label Dec 29, 2017
@stale stale bot removed the abandoned label Dec 31, 2017
@cmccandless cmccandless merged commit eccdc42 into exercism:master Jan 9, 2018
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.

4 participants