w3c / string-meta

How to add direction and language metadata to strings
https://w3c.github.io/string-meta/
11 stars 18 forks source link

Add IDL for `LanguageMap` #88

Open aphillips opened 3 weeks ago

aphillips commented 3 weeks ago

Called out by Webauthn in https://github.com/w3c/webauthn/issues/2151

emlun commented 3 weeks ago

Commenting on: https://github.com/aphillips/string-meta/commit/9624fbe632a6293c272299eb5f4d8a9f49cb4b43

I'm quite confused, because it seems like the language map examples are not consistent with each other or the LanguageMap definition, nor the LanguageMap definition with the prose descriptions:

Could you help me understand how these definitions and examples are meant to relate?

The way I interpret the intent of the prose descriptions, Example 4 matches what I would expect. I think the map syntax best expresses the intent of a collection of key-value pairs - and is easiest to work with as a developer - and it doesn't seem useful to use an explicit sequence structure for implementation efficiency, if that is the concern. Maps and sequences most likely take the same time to parse or search in anyway: in JSON, CBOR and XML a plain linear search is needed since the items don't describe their serialization length, while in ASN.1 DER the parser can "skip ahead" in a map just as easily as in a sequence. So without knowledge of any other concerns that went into this design, my expectation for a LanguageMap IDL definition would simply be a record type with language tags as keys and LanguageEntry values:

typedef record<DOMString, LanguageEntry> LanguageMap;

// Or alternatively:
typedef DOMString LanguageTag;
typedef record<LanguageTag, LanguageEntry> LanguageMap;

Either of these would neatly match Example 4 and be easy to work with as a developer.

aphillips commented 3 weeks ago

@emlun Thanks for the comments. The IDL for LanguageMap is incorrect. It should be a record as noted.

Example 14 (and nearby friends) is definitely broken, wrt being valid JSON. I'll fix that also while making the necessary changes.

emlun commented 3 weeks ago

Thanks @aphillips! The current design looks good to me.

I spotted a few more minor issues:

aphillips commented 3 weeks ago

This still uses DOMString as the key type in the record definition, but references LanguageTag in the prose description.

It's a bug in Respec. Respec reports an error because LanguageTag is not DOMString. I am considering ignoring the error.

LanguageRecord no longer exists. Typo: instead of in LanguageMap

Fixed the first, replaced the kbd tags with Respec IDL markup {{LanguageMap}}

@emlun Do you not have review permission on the PR?

emlun commented 2 weeks ago

Oh! I didn't realize there was a PR. All I'd seen was the link to this issue from https://github.com/w3c/webauthn/issues/2151, and #89 hasn't shown up in the activity feed in this thread. I'll post in the PR if I find anything else, but I think I'm done with my review for now.

aphillips commented 2 weeks ago

P.S. I added you to the acknowledgements. Appreciate the help!