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

luhn 1.0.0.2: only test isValid, use (most) x-common cases #533

Merged
merged 1 commit into from
May 9, 2017
Merged

luhn 1.0.0.2: only test isValid, use (most) x-common cases #533

merged 1 commit into from
May 9, 2017

Conversation

petertseng
Copy link
Member

@petertseng
Copy link
Member Author

Say the word if you disagree with the path taken. I don't want to have to do extra work to filter out the invalid inputs only to have to delete the work if exercism/problem-specifications#523 is accepted.

@petertseng
Copy link
Member Author

and sorry I hadn't set up hlint on this machine


it "create 232320057766355" $
create 232320057766355 `shouldBe` 2323200577663554
specs = describe "valid" $ for_ cases test
Copy link
Member Author

Choose a reason for hiding this comment

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

notice! no luhn!

-- , Case { description = "valid strings with punctuation included become invalid"
-- , input = "055-444-285"
-- , expected = False
-- }
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it seems that you suppressed one test:

    {
      "description": "valid strings with symbols included become invalid",
      "property": "valid",
      "input": "055£ 444$ 285",
      "expected": false
    },

Was it intentional?

I don't consider that a problem, because considering what was discussed in exercism/problem-specifications#428, it probably shouldn't exist anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! I guess I'll add it (and comment it) for the purpose of following x-common, but you are right that we expect the case to go away soon.

petertseng added a commit that referenced this pull request May 9, 2017
* bowling 1.0.0.2: drop unneeded rolls, edit a description
* leap 1.0.0.2: explain criteria in descriptions, put in progress order
* luhn 1.0.0.2: only test isValid, use (most) x-common cases
* phone-number 1.2.0.3: enforce area/exchange not starting with 1
* roman-numerals 1.0.0.2: add descriptions
@petertseng
Copy link
Member Author

Oh interesting. @rbasso could I trouble you to approve this one again? Since I made a change since the previous approval, and this matters if I'm octomerging. Interestingly, the fact that I made changes since the previous approval wouldn't stop me from merging on the web, which actually makes this seem a little inconsistent.

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

Successfully merging this pull request may close these issues.

2 participants