forth: Remove test for empty input

[N-Parsons]

This PR adds a new test designed to check that an empty line (represented by an empty string) is handled/ignored and leaves the stack empty without failing.

This PR removed the test for empty input [] in line with the discussion below. This test was considered to be unexpected for the current problem description.

[Insti]

This PR adds a new test designed to check that an empty line (represented by an empty string) is handled/ignored and leaves the stack empty without failing.

This requirement does not seem to be mentioned in the description which seem to be otherwise explicit about which commands should be supported.

[N-Parsons]

@Insti, I'm not quite sure that I understand your point - I wouldn't consider an empty command to really be a command, but I would expect any parser to be able to handle them gracefully, particularly when empty lines have no effect in Forth. My reasoning was that interpreters (eg. gforth) are generally tolerant of empty inputs and blank lines, and similarly, blank lines can be used within scripts without issue. I think an empty input line is best represented by an empty string.

[Insti]

I agree with you that a well behaved parser should ignore blank lines.

A solution that matches the described requirements may or may not handle blank lines gracefully, but its exact behaviour is undefined in the description and irrelevant to the rest of the exercise.

If you wish to add an additional test for this in a particular track, that is fine.
But I don't agree that the test should be added to the canonical data.

See also: #902

[petertseng]

If the description does not change: Would it therefore be appropriate to remove the test of [] as well? If so, would be a good way to repurpose the PR.

[N-Parsons]

@Insti, thanks for your clarification.

If we are going to continue to test for [] (effectively a null input?), then I think we should test for the more logical [""] (empty line). Or, as @petertseng points out, we could just remove both cases - certainly I feel that [""] is more to be expected than [] as an input. I'm happy to rework this PR to remove these two cases if you think that it would be the best solution.

Also, I noticed that the canonical data currently doesn't test for case insensitivity, despite it being an explicit part of the problem description. I'm happy to add these tests, but do you think that they would be better as part of this PR (with a suitable name change), or should they go into a different PR?

[Insti]

If we are going to continue to test for [] (effectively a null input?), then I think we should test for the more logical [""] (empty line)

That sounds sensible.
I am in favor of removing [] as a canonically tested input.
(I've not recently looked at the rest of the tests, I only saw the diff Github showed me here.)

... case insensitivity, despite it being an explicit part of the problem description

I'd recommend making a new PR. There is no shortage of available pull requests.

[Insti]

Editing the original post here with an update would be good.
Use ~~ to strikethrough the old description of the PR.

[N-Parsons]

Thanks for reminding me, @Insti - I forgot about the description!

[stevejb71]

From a TDD perspective, the first (empty list) function requires you to write a function with the desired type and name, but nothing else. The next test is a bit more complicated.

Nothing in the descriotion mentions parsing an empty string, and it may add additional difficulty.

So I'd be in favour of leaving the tests as is.

[Insti]

the first (empty list) function requires you to write a function with the desired type and name, but nothing else.

This is an implementation detail and not something that is required to verify that the solution is correct, so does not belong in the canonical data.

Students are encouraged to write their own TDD tests and I would hope by the time they got to an exercise implementing a FORTH interpreter that they would be able to do this.

Therefore removing this test from the canonical data is the right thing to do.

[ErikSchierboom]

Therefore removing this test from the canonical data is the right thing to do.

I agree.

[Insti]

Thanks @N-Parsons ❤️