-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Test schema: allow src
property to either be a string or array of strings
#923
base: main
Are you sure you want to change the base?
Conversation
…trings This makes the test schema consistent with the schema in https://github.com/unicode-org/conformance/blob/main/schema/message_fmt2/testgen_schema.json
Co-authored-by: Addison Phillips <[email protected]>
Co-authored-by: Addison Phillips <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was previously proposed, discussed, and rejected in #767 (comment). What has changed since then?
@eemeli I think we deferred the decision on how to implement the string-array To recap previous discussion, we were deciding between these two options: {
src: string | string[]
} and: {
src: string
} | {
srcs: string[]
} My preferred approach is what @catamorphism has proposed in this PR. Semantically, we're talking about a single "source" in both cases (a string array would still represent one "source") so I think it makes sense for the JSON to use a single property. My understanding is that the drawback for this approach is the extra code required to deserialize the JSON using libraries such as Gson for Java. Are there still concerns around deserialization (e.g. in unicode-org/conformance executors)? (A third option would be to make |
There's a workaround for Java that I implemented in ICU4J when I did the test unification. So I think the objection in #767 (comment) is no longer relevant (@mihnita ?) |
Ping @mihnita I'm inclined to follow what the test suite folks say here, but I don't see any approving reviews... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like the deserialization concern is addressed, plus schema consistency with unicode-org/conformance makes sense.
Why does it make sense to apply this change to make the test schemas consistent? If there is a reason for consistency, why have two different schemas to start with? I don't object to the change, I'm just getting a sense that either there ought to be one schema that works both here and for the ICU conformance tests, or that there shouldn't be a need to adjust this schema due to requirements of the ICU conformance tests. |
Per the 2024-11-11 teleconference, I'm labeling this for 47 so that it is not required for 46.1. We should resolve this issue as soon as practical, but not block on it for shipping. |
It is not for ICU, it is in general. |
Per our rules, this does not qualify for fast-track. What's more, it's not exactly fast-track if we're holding it for 47 😸 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. thanks.
This makes the test schema consistent with the schema in https://github.com/unicode-org/conformance/blob/main/schema/message_fmt2/testgen_schema.json