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

Add Employment authorization document test vector. #106

Merged
merged 5 commits into from
Jan 2, 2025

Conversation

Wind4Greg
Copy link
Contributor

@Wind4Greg Wind4Greg commented Dec 17, 2024

This informative PR adds a more elaborate and realistic test vector based on an employment authorization document example.


Preview | Diff

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Just lowering the number of sequential nouns used to identify things.

}
},
"name": "Employment Authorization Document",
"description": "Example Country Employment Authorization Document.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"description": "Example Country Employment Authorization Document.",
"description": "Employment Authorization Document for Example Country.",

Copy link
Member

Choose a reason for hiding this comment

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

or

Suggested change
"description": "Example Country Employment Authorization Document.",
"description": "Example Employment Authorization Document for Country.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TallTed, @dlongley or @msporny I took the unsigned inputs from the VC playground with some modifications (removed render method, substituted single pixel image for long PNG). Any changes to the input JSON file requires rerunning test vector generation code and changes derived/generated test vectors. Can folks come to an agreement on any changes to inputs (or not) then I will rerun the vectors.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with changing this description as @TallTed recommends.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well just simplify to Example Employment Authorization Document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TallTed are you okay with @dlongley suggestion or your original suggestion. Let me know then I'll rerun the code to generate the test vectors.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the background on the language there is it started as "[Country Name] Employment Authorization Document". Some other examples we have would use "Utopia" there. Real country names would be similar. It wasn't originally intended to be "Example [Country Name] EAD", but I see how it looks like that now and is a bit awkward. It's only an example so hopefully anyone that copies whatever ends up here will adjust as needed.

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with @dlongley's simplification. It might be better to quote the identifier of the thing being modified by "Example", as --

Suggested change
"description": "Example Country Employment Authorization Document.",
"description": "Example \"Employment Authorization Document\".",

}
},
"name": "Employment Authorization Document",
"description": "Example Country Employment Authorization Document.",
Copy link
Member

Choose a reason for hiding this comment

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

as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TallTed so this ends up being no change. Is this what you wanted?

Copy link
Member

@TallTed TallTed Dec 19, 2024

Choose a reason for hiding this comment

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

so this ends up being no change. Is this what you wanted?

I don't understand how it ends up being no change. All the earlier comments on this PR have what I believe to be editorial changes, reducing strings of 4, 5, 6 nouns to 3, 2, 1 noun, to ease comprehension by non-native English speakers and by various software tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TallTed I don't understand what the previous "as above" comment meant. I can rerun the test vector computation only if I know what to set the inputs to be. I need you and @dlongley to agree. Otherwise please recompute the test vectors that correspond to your changes and I can approve.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with whatever @TallTed wants as a description here.

Copy link
Member

@TallTed TallTed Dec 19, 2024

Choose a reason for hiding this comment

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

@Wind4Greg — The gist is "don't use a sequence of 3 or more nouns to identify something". Doing this makes comprehending and using the document more difficult for non-native English speakers (and even many native English speakers), as well as those using various accessibility tools. This is an example:

Suggested change
"description": "Example Country Employment Authorization Document.",
"description": "Example \"Employment Authorization Document\" for Country.",

I'll go make specific suggestions where I had earlier said "as above". I do all my work through GitHub's web interface, so I cannot compute the text vectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go with "description": "Example Employment Authorization Document" extra escape sequences are confusing to implementers.

index.html Outdated Show resolved Hide resolved
@msporny
Copy link
Member

msporny commented Jan 2, 2025

Editorial, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 8fe612f into w3c:main Jan 2, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants