Skip to content

(WIP) forth: implement#753

Merged
ilya-khadykin merged 13 commits intoexercism:masterfrom
cmccandless:implement-forth
Oct 14, 2017
Merged

(WIP) forth: implement#753
ilya-khadykin merged 13 commits intoexercism:masterfrom
cmccandless:implement-forth

Conversation

@cmccandless
Copy link
Contributor

@cmccandless cmccandless commented Oct 6, 2017

Resolves #739

TODO:

  • add README
  • add forth to config.json
  • create solution template forth.py
  • write test cases forth_test.py
  • write example solution example.py

Copy link
Contributor

@N-Parsons N-Parsons left a comment

Choose a reason for hiding this comment

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

It would be best to avoid using input as an argument in your example.py since input is a built-in function and this can cause confusion.

It would also be preferable for your example.py to use named exception types in your try: ... except: ... blocks rather than just catching all possible exceptions.

I think that the tests would be better implemented using exceptions rather than returning None. In gforth, most of these errors would raise a "stack underflow", so we could define a custom StackUnderflow exception. Division by zero could raise a ZeroDivisionError and other errors would probably be fine with ValueError.

"core": false,
"unlocked_by": null,
"difficulty": 10,
"topics": [
Copy link
Contributor

Choose a reason for hiding this comment

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

We've switched to snake_case naming convention for topics to increase consistency among tracks, so could you please choose them from the following list - https://github.com/exercism/problem-specifications/blob/master/TOPICS.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Topics renamed to conform to naming convention.

Copy link
Contributor

@ilya-khadykin ilya-khadykin left a comment

Choose a reason for hiding this comment

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

Overall it looks good 👍
I've left some comments

@N-Parsons pointed to some issues, I think they should be addressed as well

from forth import evaluate


class ForthAdditionTest(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these test cases adopted from https://github.com/exercism/problem-specifications/blob/master/exercises/forth/canonical-data.json?
If so, please state that in the comment denoting version as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they were adapted from the canonical data. Comment with test data version has been added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget that from example import ... needs to be changed back to from forth import ....

"uuid": "e348a307-078c-5280-65af-a159283d4e79438b755",
"slug": "forth",
"core": false,
"unlocked_by": null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why difficulty is set to maximum value, it should be lower

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Difficulty lowered to 5.

@@ -1,8 +1,12 @@
class UnderflowError(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor point, but I think this might be better as a StackUnderflowError to make its meaning a bit more clear and to differentiate it from arithmetic underflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you put it that way, I agree. Original intent was to match convention, since there is an existing OverflowError.

Copy link
Contributor

@N-Parsons N-Parsons left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@ilya-khadykin ilya-khadykin left a comment

Choose a reason for hiding this comment

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

👍

@ilya-khadykin ilya-khadykin merged commit 29561eb into exercism:master Oct 14, 2017
@ilya-khadykin
Copy link
Contributor

Thanks for your work and congrats on implementing this one 👍, @cmccandless

@ilya-khadykin
Copy link
Contributor

@cmccandless, one more thing: will you please remove WIP tag when you are done with the PR to avoid confusion in future PRs?

@cmccandless cmccandless deleted the implement-forth branch October 17, 2017 17:05
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