-
Notifications
You must be signed in to change notification settings - Fork 20
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
added human readable/machine readable, changed example to be more clear. #138
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0e8a514
added human readable/machine readable, changed example to be more clear.
mkhraisha 89ab44f
Update index.html
mkhraisha a209b44
Update index.html
mkhraisha f0e2f83
revert example changes
mkhraisha c95f903
Update index.html
mkhraisha 86fd178
Update index.html
mkhraisha File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think this change isn't necessary? Machines can, of course, read the
status
field. The key thing is that it is a string representation of a hexadecimal value. It's maybe a little curious that you don't specify the format? The examples shows e.g.0x0
or0x1
, butDEADBEEF
,-1e
, or10EF
are "hex strings" too. Perhaps: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.
@mprorock @msporny correct me if i'm wrong, but I believe
0x1
was just an example, and we do not intend to limit hex values to be prefixed with0x1
.my understanding of current consensus is that this is a predifined list of status codes that the issuer is responsible for providing a definition for.
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.
There is no consensus on this feature outside of the Traceability cohort (other implementers aren't implementing it; we're putting it in there for Transmute, Mesur.io, and Mavennet).
You guys will need to implement this and tell us how you intended to use this feature.
IIRC, "0x1" wasn't an example, it was intended to express a hexadecimal value in a very specific format. I was presuming that the "message" field contained information that was useful to a developer, but MUST NOT be conveyed to an end-user (which seems to be what we're back to in this PR).
Please check w/ the Traceability folks to help us understand how they want to encode and use the
status
field and themessage
field. I expect that something like this will be acceptable to i18n:"status": "0x1", "message": "frobinator_error"
but something like this will cause i18n issues (as it should, because it's not clear that the
status
value is a hexadecimal value and thatmessage
is not meant to be displayed to end users:"status": "deadbeef", "message": "ERROR: The frobinator is no longer connected to the deviator."
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.
we utilize hex codes most often, however, we are open to a definition that does not block use of a hex code of the index - this is NOT user facing information
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.
w3ctag/design-principles#453
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.
@OR13 @mkhraisha @mprorock ping again -- trying to get closure on this issue so we can have a clear path to Candidate Recommendation in two weeks.
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 feature is required by the traceability cohort.
We should not restrict it to hex codes however.
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.
Ok, got it, so here's where we are as of right now:
status
value.As such, we're just going to go with all that, merge this PR (with @aphillips additions), and call it a day.
If you want a packed bitfield structure, you're going to have to use a binary representation of some kind. I don't think we can leave this open ended and hope for interop on that requirement from Traceability.
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.
@mkhraisha looks like you didn't allow Editors to modify your PR. I'm going to merge what you have and apply @aphillips' changes on top of that.
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.
Applied @aphillips changes in 6626331.