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

Optional Feature Test Vectors #160

Merged
merged 7 commits into from
Apr 28, 2024
Merged

Conversation

Wind4Greg
Copy link
Collaborator

@Wind4Greg Wind4Greg commented Apr 19, 2024

This PR provides a full set of test vectors for the option features of (1) Anonymous Holder Binding, (2) Pseudonym with Issuer-known PID, and (3) Pseudonym with Hidden PID. It also partially addresses issues: #156, #153, and #150.

Note that all test vectors sections make extensive use of the ReSpec data-include feature which in the past led to the contents of the test vectors not showing up in "Preview" or "Diff" modes.


Preview | Diff

@TallTed
Copy link
Member

TallTed commented Apr 19, 2024

If at all possible, it would help to split giant PRs like this (4300 lines changed across 65 files) into one per (still large) commit. Even just reviewing human-facing language, this scale poses a challenge.

@Wind4Greg
Copy link
Collaborator Author

Hi @TallTed except for the index.html file, most of the 65 files are machine generated and are used by developers in testing and debugging. So reviewing the index.html file is of the primary editorial concern. We get feedback from developers on the rest of the files as they implement the spec and test against them. Also these other files tend (the *.json) files tend to get updated as dependent specs such as BBS, @context files, and such change.

@TallTed
Copy link
Member

TallTed commented Apr 22, 2024

most of the 65 files are machine generated

That should be flagged somewhere, somehow, in PRs, so that reviewers can review only the file(s) upon which the machine-generated are based, along with whatever scripts or other things are processed to produce those machine-generated docs, and ignore the machine-generated (or at least, review the machine-generated knowing that they're based on other things, which are where any issues with the machine-generated must be fixed).

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
… and grammar improvements.

Co-authored-by: Ted Thibodeau Jr <[email protected]>
@Wind4Greg
Copy link
Collaborator Author

Thanks @TallTed for the edits. I'm not sure how to "flag" the machine generated files other than putting them in a directory marked TestVectors along with subdirectories that further indicate what use case or feature they belong too. Ideas?

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

I haven't checked these test vectors, but +1 for getting them in to make it easier for implementers to start converging.

@TallTed
Copy link
Member

TallTed commented Apr 23, 2024

I'm not sure how to "flag" the machine generated files other than putting them in a directory marked TestVectors along with subdirectories that further indicate what use case or feature they belong too.

That's a fine way.

Any means by which "machine-produced artifacts of CI or other automated process, based on human-edited documents" can be easily distinguished from "human-edited documents that are drawn on by automated processes, to deliver machine-produced artifacts" -- such that reviewers (like me) who can't tell them apart at a glance don't waste our time editing the former set (though inspecting them might still be worthwhile, as that can expose issues in the automations as well as in the human-edited source docs).

@msporny
Copy link
Member

msporny commented Apr 28, 2024

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

@msporny msporny merged commit 31cacd2 into w3c:main Apr 28, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants