Make expErrors use an array (and only an array)#1076
Make expErrors use an array (and only an array)#1076aphillips merged 12 commits intounicode-org:mainfrom
Conversation
mihnita
requested review from
aphillips,
eemeli and
mradbourne
and removed request for
eemeli
| "bidiIsolation": "none", | ||
| "locale": "en-US", | ||
| "expErrors": false | ||
| "expErrors": [] |
There was a problem hiding this comment.
In fallback.md you left out expErrors. Here you supply an empty array. I think we should probably omit this one?
There was a problem hiding this comment.
@aphillips It's required in this file otherwise some tests would be left without an expectation (e.g. lines 44-55). This would also be the case in time.json.
As Eemeli mentioned in his review comment, I think we need to keep the expErrors in fallback.md.
There was a problem hiding this comment.
I think we are talking about fallback.json, not fallback.md?
I updated fallback.json and added errors for each individual case, instead of doing it in defaultTestProperties
There was a problem hiding this comment.
For date.json, datetime.json and time.json I added "expErrors": [] to defaultTestProperties because the schema says that entries are required to have one of exp, expParts or expErrors
In these 3 files we had tests with src only.
So I had to add a expErrors (not enough info for exp, expParts, exact date/time formatting not part of the spec)
aphillips
changed the title
eemeli
left a comment
eemeli
left a comment
There was a problem hiding this comment.
Some fixes, may need a second pass later to make sure I caught everything.
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
|
need to change bad operand to bad option (2025-06-16) |
|
In 2025-06-30 call, we agreed to merge this once @eemeli has reviewed positively. Nudging him gently with this comment. |
eemeli
left a comment
eemeli
left a comment
There was a problem hiding this comment.
There's a test that also needs to be updated to use bad-option.
test/tests/fallback.json
Outdated
| "exp": "{|42|}", | ||
| "expParts": [{ "type": "fallback", "source": "|42|" }] | ||
| "expParts": [{ "type": "fallback", "source": "|42|" }], | ||
| "expErrors": [{ "type": "bad-operand" }] |
There was a problem hiding this comment.
| "expErrors": [{ "type": "bad-operand" }] | |
| "expErrors": [{ "type": "bad-option" }] |
There was a problem hiding this comment.
Hmm... I found this confusing until I remembered that we agreed to use bad-option to avoid creating a label for message-function-error in the not at all official definition under tests here. Non-WG members won't have the benefit of having been on the telecon when we discussed it 😉
Unlike the other classes of error, MFE can have implementation-defined errors. We should make this clearer in the test suite text so that implementations do not merely ape our selection of bad-option when an internal exception/error occurs. I think it would be okay to commit this, but we need to put more effort into the text in the /tests page to make super clear that bad-option is not required by non-test functions.
There was a problem hiding this comment.
Agreed on all counts. I also noticed while implementing this change that we lack a generic error for failures in formatting or selection, which could be due to the operand, options, or the phase of the moon. As in, values for which resolution succeeds, but then the MessageValue.formatToString() or similar call throws an error. It's pretty likely to involve some bad option, but that's not actually guaranteed -- and beyond the scope of this PR.
Co-authored-by: Addison Phillips <addisonI18N@gmail.com>
5 participants
Fixes issue #993