Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

catamorphism
Copy link
Collaborator

test/schemas/v0/tests.schema.json Outdated Show resolved Hide resolved
test/schemas/v0/tests.schema.json Outdated Show resolved Hide resolved
@aphillips aphillips added fast-track Non-spec editorial changes, etc. LDML46.1 MF2.0 Draft Candidate labels Nov 4, 2024
Copy link
Collaborator

@eemeli eemeli left a 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?

@mradbourne
Copy link
Collaborator

@eemeli I think we deferred the decision on how to implement the string-array src because we didn't have an immediate use-case.

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 src a string[] in all cases but I don't feel there is a need to add more [] to the test JSON because the single-string values are more common.)

@catamorphism
Copy link
Collaborator Author

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 ?)

@aphillips
Copy link
Member

Ping @mihnita

I'm inclined to follow what the test suite folks say here, but I don't see any approving reviews...

Copy link
Collaborator

@mradbourne mradbourne left a 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.

@eemeli
Copy link
Collaborator

eemeli commented Nov 10, 2024

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.

@aphillips aphillips removed the LDML46.1 MF2.0 Draft Candidate label Nov 11, 2024
@aphillips aphillips added the LDML 47 LDML 47 Release (Stable) label Nov 15, 2024
@aphillips
Copy link
Member

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.

Comment on lines +63 to +65
".match $y 1.0",
"{{1.0}} 1",
"{{1}} * {{bad-option-value}}"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These line splits are weird. Is that intentional?

Comment on lines +100 to +101
".match $x $y",
"1 1 {{1,1}}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This results in a data model error. The same applies to all further array-of-strings sources as well.

@mihnita
Copy link
Collaborator

mihnita commented Nov 18, 2024

adjust this schema due to requirements of the ICU conformance tests.

It is not for ICU, it is in general.
ICU can parse standard json, same as everybody else.
Being able to split a string on multiple lines is for the readability of the json and has nothing to do with ICU.

@aphillips aphillips removed the fast-track Non-spec editorial changes, etc. label Nov 22, 2024
@aphillips
Copy link
Member

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 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LDML 47 LDML 47 Release (Stable) test-suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants