More validation in `stringToNumber()` #917

[knod]

I don't think this will interfere with the linting, but if it does, we can save it for later.

[turnerhayes]

Can you explain a little more about the motivation behind this change? Why throw instead of returning NaN?

[knod]

Because NaN fails silently and I'm not a fan of failing silently - I might not notice that I've messed up somewhere and once I do figure it out, I'll have a harder time tracking down where the problem's coming from.

Is there a reason NaN would be a desired result?

[turnerhayes]

Not particularly; I'm always just a bit cautious about using exceptions, and wanted to get a clear idea why they were used here. Presumably, this is intended to be used where a non-number argument would be an error case? For example, is this used on user-entered data?

[knod]

It is meant to be used on user-entered data and I don't believe any inputs return numbers. We could make it more flexible and allow numbers to come in, but I think that would be a different PR.

[turnerhayes]

Personally (and this is largely a subjective preference), I think it looks nicer to compare against NaN than to do a try/catch or let the error go through. If the error message is displayed to the user via the UI, then that's another matter, and having more specific errors like you have here ("this shouldn't be empty"/"this should be a valid number") might be more useful than simply checking against NaN. But in that case, I'd suggest changing the error message terminology.

[dylanesque]

I'm iffy about this one. The logic in the Shouldn't this be handled by validation in the forms themselves that prevent the user from passing in anything other than a number of 0 or greater?

[knod]

@turnerhayes : What do you mean by 'compare against NaN'? Maybe some example code?

The error isn't meant for the user, though, it's meant for developers. If it happens to the user, they'll get the option to send feedback to us about an error happening, which should contain the error stack and, if the user is willing, also the client data.

Have I understood the concerns?

[knod]

@dylanesque : This isn't validation. This is converting a string into a number so the logic can do math with it. It gets stored as a number.

[knod]

This just makes this particular function more robust and communicative to those coding.

[dylanesque]

I'm pretty skeptical as to why the user should be able to convert strings (unless accessibility, I guess?) as opposed to being forced to input valid numbers only, but "too much testing" is not the side of that discussion I really want to fall upon.

[knod]

@dylanesque : This isn't for the user. They won't be converting anything. HTML inputs only give back string values, even when people put in numbers. Even when it's an input with type="number". We're going to have to convert strings to numbers as we've been doing the whole time. This is just an improvement of a function that already existed.

[knod]

@turnerhayes : Did that reply up there answer your question?

[turnerhayes]

When an invalid input is entered, there are basically two ways to handle that:

1. Throw an error, and catch it in the code that runs stringToNumber() (and do something reasonable in the UI)

   let numValue;
   try {
   numValue = stringToNumber(userInput);
   }
   catch (ex) {
   // do something about the error
   }

2. Return NaN to indicate that the input was invalid, and, in the code that runs stringToNumber(), check if the return value is NaN (and do something reasonable in the UI)

   
   const numValue = stringToNumber(userInput);

if (isNaN(numValue)) { // do something about the error }

Both have their pros and cons; from a philosophical standpoint, I'm generally inclined to treat exceptions as exceptional circumstances (e.g. I/O unavailable, out of memory, etc.), and invalid user input doesn't really fit that, IMO.

One of the advantages to approach 1 is that it lets you distinguish between different reasons for error that approach 2 doesn't (through error messages or different error classes). But if the consuming code doesn't do anything with the specific error reason, then that benefit doesn't really apply.

Does that help?

[turnerhayes]

On further reflection, I'm rethinking my position; the question is really, "when do we use exceptions?" and "in exceptional cases" doesn't really help. I think the real answer is "when the code doesn't know the correct way to handle the case and wants to leave it up to the caller to figure out", and I think this probably fits that case. If the function knew what to do with invalid inputs, we could have it do that (e.g. return 0 if invalid), but this function doesn't know that.

I'm okay with this API change.

[turnerhayes]

That being said, it's probably worth revisiting how the consuming code deals with invalid inputs--do we want it to just error out on invalid input? Show something in the UI? Revert to previous value? etc. Right now, we are (presumably) introducing uncaught exceptions where there were none before, and I'm not sure that's a state we want to be in.

[dylanesque]

On further reflection, I'm rethinking my position; the question is really, "when do we use exceptions?" and "in exceptional cases" doesn't really help. I think the real answer is "when the code doesn't know the correct way to handle the case and wants to leave it up to the caller to figure out", and I think this probably fits that case. If the function knew what to do with invalid inputs, we could have it do that (e.g. return 0 if invalid), but this function doesn't know that.

I'm okay with this API change.

From what I know about exceptions, you don't want to throw them unless it's for something deeply unusual, and I don't think a type error meets that metric. This PR confuses me because it reads as defensive coding for validation/error handling that isn't on the client side.

[knod]

Welp, that's what you get for making assumptions about tests.

[knod]

Which is not any fault of the test writers. We've written lots of tests, we just haven't gotten far enough to be this thorough.

[knod]

"when the code doesn't know the correct way to handle the case and wants to leave it up to the caller to figure out"

Exactly

[knod]

@dylanesque :

unless it's for something deeply unusual

It shouldn't be, actually. There should actually be more validation to help people using the code more easily understand when/how their edits are going wrong. It's just hard to write thorough code so it's not done a whole lot. In this case, the way people currently tend to write code isn't a good indicator about how it should be written.

defensive coding for validation/error handling that isn't on the client side

That's exactly what it is, but that's not necessarily a bad thing. Validation handling and defensive coding isn't just for the client side.