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

forth: Expect a list rather than a string or Text #412

Merged
merged 2 commits into from
Nov 7, 2016
Merged

forth: Expect a list rather than a string or Text #412

merged 2 commits into from
Nov 7, 2016

Conversation

petertseng
Copy link
Member

Like in exercism/problem-specifications#394, I request that we
question our assumptions. Why have we requested a string? Well, it was
like that in exercism/exercism#1188, the
first appearance of this exercise. But we can just use a list, right?

Copy link
Contributor

@rbasso rbasso left a comment

Choose a reason for hiding this comment

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

Besides one small change needed in the stub solution, everything seems perfect. 👍

You can take the opportunity to update this exercise to the "new proposed format" if you wish to provide a clean package.yaml, without a dependency on containers.

@@ -23,5 +23,5 @@ empty = undefined
evalText :: Text -> ForthState -> Either ForthError ForthState
evalText = undefined

formatStack :: ForthState -> Text
formatStack = undefined
toList :: ForthState -> Text
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you forgot to change this line to [Int].

@petertseng
Copy link
Member Author

provide a clean package.yaml, without a dependency on containers.

Sure! I will take the opportunity to do so.

I think that you forgot to change this line to [Int].

Oh dear, maybe I need to speed up the process of doing the "should-compile-but-fail-tests" and we should throw every stub solution into that category. Would prevent this sort of mistake.

Like in exercism/problem-specifications#394, I request that we
question our assumptions. Why have we requested a string? Well, it was
like that in exercism/exercism#1188, the
first appearance of this exercise. But we can just use a list, right?
This means our package.yaml no longer presents `containers` to students.
@petertseng
Copy link
Member Author

okay, example is moved - note that I didn't know what to put after success- so I put... success-standard. Maybe just success-example? Or maybe simply success will do?

Also, maybe I should take a moment to do this for all package.yaml that have extra packages...

source-dirs: src
dependencies:
- containers

Copy link
Contributor

Choose a reason for hiding this comment

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

Great...you even remembered to remove the comments. :)

@petertseng
Copy link
Member Author

petertseng commented Nov 7, 2016

OK, test passed which is what I was waiting for. If we have no better suggestion than success-standard then I'll merge this one (or you can).

@rbasso
Copy link
Contributor

rbasso commented Nov 7, 2016

success-standard is good, don't worry! 👍

@rbasso rbasso merged commit 27b9f71 into exercism:master Nov 7, 2016
@petertseng petertseng deleted the forth branch November 7, 2016 01:55
petertseng added a commit to exercism/rust that referenced this pull request Jun 7, 2017
Questioning the assumptions: Why must the output be a string?

Reasons I thought of:

* It was this way in the original version of the exercise:
  exercism/exercism#1188
    * But, we have no obligation to follow that.
* It matches the fact that we have strings as inputs.
    * But, it is probably more work for the student to create a string.
    * It can be argued that it's easier to check the vector.

Previous discussions:
exercism/problem-specifications#394 (comment)
exercism/haskell#412

This doesn't get this track all the way to forth 1.0.0, but is a useful
step there.
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.

2 participants