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

Representation of undefined/exceptional values in canonical-data.json #401

Closed
Insti opened this issue Oct 9, 2016 · 13 comments
Closed

Representation of undefined/exceptional values in canonical-data.json #401

Insti opened this issue Oct 9, 2016 · 13 comments

Comments

@Insti
Copy link
Contributor

Insti commented Oct 9, 2016

The current instructions in the readme recommend using -1 to indicate an exception.

expected: the expected result (this would be -1 when we expect an exception)

Is '-1' the best value to be using? I think that null would be a better value to use to indicate an exceptional or undefined result.

Discuss.

@dvberkel
Copy link

dvberkel commented Oct 9, 2016

I don't think null is a very language neutral value to indicate an undefined or exceptional result. Some languages do not have a null value. And for languages that do, I would personally go to great lengths to avoid returning null, as it promotes a bad practise.

Furthermore I think that undefined and exceptional values are representing two different things. Something can be undefined, without being an exception. I would opt for having different conventions for the two cases

@Insti
Copy link
Contributor Author

Insti commented Oct 9, 2016

It is up to the language tracks to implement undefined and exceptional values in the most appropriate manner for their language.

Using a null value in the json does not mean that the language needs to use null.

@dvberkel
Copy link

dvberkel commented Oct 9, 2016

Using a null value in the json does not mean that the language needs to use null.

That is certainly true. But I still have the same objections. Let's say that someone wants to port an exercise to a language with null. If that person looks at the canonical data and sees a null, they could assume that it is an actual requirement to return an explicit null. You would have to know about the convention that null doesn't mean null but either throw an exception or signal/provide an undefined answer, which ever is appropriate for the target language.

I would rather see that the JSON is more self contained. That is doesn't rely on conventions and mores, but that it makes sense on its own.

@IanWhitney
Copy link
Contributor

I think this is a "Tabs vs Spaces" thing. It depends. As long as the description in the JSON file describes what's expected, I think it's fine.

@rbasso
Copy link
Contributor

rbasso commented Oct 11, 2016

The C programmer that still lives inside of me thinks that -1 is perfect.
The Haskell one is screaming that this is so $&#*<^%... well, let's just say that he favors null but misses Maybe.

The problem with using -1 is that we are encoding an error as a standard value, and this makes it impossible to distinguish it from a valid negative value.

Of course, there is the risk of someone misinterpreting a null, as @dvberkel mentioned, but there is the same risk with -1.

At least null is a special value in JSON...

@petertseng
Copy link
Member

I mostly care about this when a language that wishes to write an automated test generator expects that all values at, say, expected are all the same type. Mixing some non-numeric type and -1 would cause trouble then, possibly causing the generator of that language to be unable to parse the JSON file! Then again, null could cause trouble as well for the same reason.

I don't have any specific recommendations to make since I'm not working on any generators right now. But I do want to point this out since I want our future designs to be as amenable to generators as possible.

@jtigger
Copy link
Contributor

jtigger commented Oct 13, 2016

The JSON spec is silent on this issue. It's basically leaves to the parties involved to agree on some convention (insert @IanWhitney 's comment here).

JSON Schema essentially relies on a set of JSON keys that are treated like keywords (e.g the "type" key in { "type": "string" } or "enum" key in { "colors": { "enum": [ "red", "green", "peachpuff"...] } }).

JSON-based APIs rely on a similar scheme: the presence of an "error" key implies that the object represents an error/exception.


Straw man:

For test cases where an exception/error is expected, represent this as thus:

{
  "description": "garbage input",
  "input": "100$#^0002",
  "expected": {
    "error": {
      "message": "Invalid input."
    }
}

...where the presence of such an object implies that an error or exception should be expected by the corresponding test case. The value of "message" is not to be taken literally.

@ErikSchierboom
Copy link
Member

I like @jtigger's solution best: encode the error case as a completely different object. That makes the -1 and null are actually valid values problem completely go away. If I did have to choose, my preference would be to use null.

@jtigger
Copy link
Contributor

jtigger commented Jan 23, 2017

To be useful for test generators, we could agree on this exact shape:

An object containing exactly one property named error which contains a string.


(edit: simplified suggestion in light of #505)

@stevejb71
Copy link
Contributor

Would an error string indicate that the test should check for an exception (or Either return value in languages like haskell) with the given message?

@petertseng
Copy link
Member

petertseng commented Jan 27, 2017

Would an error string indicate that the test should check for an exception (or Either return value in languages like haskell) with the given message?

I think I agree with the first part (check for an exception, or Either, or Result, or whatever is appropriate), but not with the second part ("with the given message")

I was thinking about

The value of "message" is not to be taken literally.

If that means what I think it means, I think I agree. I generally think it is strange to test for an exact string in an error message, since it leaves a risk that the tests will break if I need to change the string.
But then you ask, how do I check that the code is giving back the correct error? Well, if strings are involved, I'd be OK with testing that the string contains some essential part of the error. Gives back a little bit of risk of breakage, but gives a bit of assurance.

Or if the language in question has discriminated variants, I could have something like type error = WindowTooLarge | WindowNegative | InputContainsNonDigits and see that the error is the correct one of those three variants. The display layer can choose how to present those errors to the user separately from the tests that check we're giving back the right error.

Okay, you say, those considerations are more important when writing code that may need to change from time to time and present error messages to its users, but does it translate to Exercism?

Well I do like to instill habits that I think are representative (obvious caveat that what I think is representative might not actually be!). But I can tell you what the tracks that I'm involved in tend to do:

  • Go: check that the error is non-nil, and that it is indeed of type error, don't check the value of Error()
  • Haskell: check Data.Either.isLeft, don't check the value contained in the Left.
  • Rust: check Result#is_err and don't check the value contained in the Err.

@petertseng
Copy link
Member

I thought interested parties should know that the proposal:

An object containing exactly one property named error which contains a string.

Is being used in #336 and thus #551.

@petertseng
Copy link
Member

Oh, honestly I should have marked #551 as "Closes #401", since the latest proposal in here has been made official... though note that null is a separate possibility from {"error": ...} for cases where it makes a difference.

petertseng added a commit that referenced this issue Nov 11, 2017
all-your-base 2.0.0

This uses the error object defined in
#401 (comment)
and thenceforth added to the README's **Test Data Format**.

In #280 we made
the explicit choice to leave various cases `expected: null` where tracks
can make their own decisions (leading zeroes, the representations of
zero, whether empty sequence is acceptable as input).

However, since `expected: null` is also used for cases where there is
always an error (and there is no decision being left to the tracks),
it becomes unreasonably hard to tell cases in these two categories
apart.

To make this easier, all cases in the latter category (always are
errors) can canonically be marked as such.
petertseng added a commit that referenced this issue Sep 24, 2018
change 1.3.0

As proposed in
#1313
#905 (comment)

In contrast to the proposal in
#336 (comment)

Although -1 is a sentinel value, the sentinel value had been overloaded
in this JSON file to mean two separate conditions:

* "Can't make target" (ostensibly, we might be able to make the target
  with a different set of coins)
* "Target is negative" (no matter what coins we provide, this target is
  always invalid)

To make clear that these two conditions are different, we use an error
object describing each.

This error object was defined in
#401

Note that this commit is not a decree that all languages must represent
these conditions as errors; languages should continue to represent the
conditions in the usual way for that language. It is simply a
declaration that these two conditions bear enough consideration that
we'll represent them with a different type and also clearly
differentiate between the two.

Closes #1313
Checks the related box in #1311
petertseng added a commit that referenced this issue Sep 24, 2018
binary-search 1.2.0

As discussed in
#1312

Although -1 is a sentinel value, using this sentinel value is not the
usual course of action in some languages. In using an error object, we
avoid giving the wrong idea that we are requiring the use of the
sentinel value.

This error value was defined in
#401

Of course, languages that wish to use a sentinel value may continue to
do so; this commit is not intended to decree that sentinel values are
forbidden.

Neither is this commit decreeing that all languages must represent this
condition as an error; it is simply a declaration that this condition
bears enough consideration that we'll represent it with a different
type.

Closes #1312
Checks the related box in #1311
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants