[ONNX] clarifies error message for upsample interpolation mode

[bjacobgordon]

@zjgarvey How's it look?

[zjgarvey]

I'd remove the quotes.

[bjacobgordon]

I was on the fence about that, too!

I encountered two contending DX considerations:

• Maintaining string "greppability"
  • tough when escaping straight double quotes like "\""
• Conveying (in an intuitive way) that the input is a string literal, even in an error message
  • using curved opening/closing double quotes somewhat solves this problem, but might add more cognitive load than it's worth.

Solution could be a raw string literal.

R"(Expected valid interpolation mode: "nearest" | "linear")"

What do you think?

[zjgarvey]

I think it would be best to remove the quotes. These match failure messages should be short and easy to search for, that way people can find the source of the match failure in the code easily.

They don't need to be fully descriptive, but the change in tone from "unsupported" to "unexpected" is helpful.

We likely created the linked issue because someone was skimming for match failures on attribute checks and saw the word "unsupported".

[bjacobgordon]

@zjgarvey I split the commit into two to narrow the scope of the convo!

Earlier, you had said:

These match failure messages should be short and easy to search for, that way people can find the source of the match failure in the code easily.

Let's quantify the change:

• number of characters added to code for message: 8
• number of characters added to message: 4

Zach's Criteria:

• short: I agree! Luckily, this only adds 4 characters to the length of the message itself
• easily searchable: I agree! And devs will still be able to copy the message as is and find the instance

  Expected valid interpolation mode: "nearest" | "linear"

Jacob's Criteria

• communicate type where possible: looks more like string literal
• inherently connected to the literals they describe:
  • searching "nearest" with the quotes and all will pull up the usage and the documentation-via-error-message!

I think these are compatible sets of criteria 😎

You had also mentioned:

[Match failure messages] don't need to be fully descriptive

Situations where a fully descriptive error message might be undesirable:

1. Security Concerns: exposing sensitive information
2. Users Experience: For end-users who may not have technical expertise
3. Performance: could be impacted by massive message
4. Redundancy: the error is common and well-documented
5. Context: Additional details might be unnecessary when the errors are self-explanatory

Our Situation

1. No sensitive info
2. No technical content added
3. 4-character diff likely has negligible performance impact
4. I didn't see any similar instances; it's a big code base haha
5. My guess is that, because you have a ton of context already, it feels self-explanatory, yeah?

In general, things I've found useful to ask myself:

• "What tiny tweak (on its own) would have made this issue easier to fix or entirely avoidable?"
• "What set of first principles were forgotten or violated that made this tougher than it coulda been?"

And those questions lead me to "oh, we could just add some quotes around the words that correspond to the string literals!"

Thanks for reading all that haha. What do you think, does any of that track?