Skip to content

assert that eval() returns ok where it should#296

Merged
petertseng merged 1 commit intoexercism:masterfrom
gsauthof:forth-check-eval
May 30, 2017
Merged

assert that eval() returns ok where it should#296
petertseng merged 1 commit intoexercism:masterfrom
gsauthof:forth-check-eval

Conversation

@gsauthof
Copy link
Copy Markdown
Contributor

This detects failed attempts where the stack looks as expected but
eval still returns the wrong result.

Also, this should point out some implementation errors more clearly
because you see a specific error result and not just the mismatching
stack content. Especially when there are multiple eval() calls in a row.

@petertseng
Copy link
Copy Markdown
Member

I think I'm for this. I would like to see the #![allow(unused_must_use)] on line1 removed too if this is going to happen, please

This detects failed attempts where the stack looks as expected but
eval still returns the wrong result.

Also, this should point out some implementation errors more clearly
because you see a specific error result and not just the mismatching
stack content. Especially when there are multiple eval() calls in a row.
@gsauthof gsauthof force-pushed the forth-check-eval branch from da13d03 to c7f395c Compare May 28, 2017 20:17
@gsauthof
Copy link
Copy Markdown
Contributor Author

Good point. The first line with that warning suppressing attribute is gone now.

(And, as expected, because of the new asserts no warnings are emitted.)

Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Thanks. I am for this and I agree with the reasoning.

Once 48 hours have passed since it was first opened (<12 hours from now) I'll merge if nobody else has any else to say.

It is interesting that we did not use the same reasoning in bowling (#213). Should we?

@petertseng petertseng merged commit 3a4949e into exercism:master May 30, 2017
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