-
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
Conversation
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
I was afraid that this was going to be the answer. Understand that this will trigger i18n requirements on the "message" field and will require explanatory text wrt. how one supports i18n on the field. You'll want to do two things at this point:
I'm tagging @aphillips and @r12a to review this PR given that they'll be able to let us know if we're hitting the mark with this PR or not. |
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.
If we're making the string human-readable now, we're going to have to provide i18n guidance.
This change undoes the rough "i18n okay-ish" aspect of the current design and is probably more work than you think it is. The value of You especially don't want to localize the string so that recipients can use the value in code to look up what the error is, which means guaranteeing that it's not free text. Admittedly, the If we make the string look like and behave like natural language, then we should take the next step and make it localizable (which might mean removing the message part from the |
@aphillips wrote:
editor hat on -- No, for all the reasons you mentioned; I advised against this sort of PR. If this is going to undo the i18n horizontal review approval, we don't want to go down that route unless the implementers feel very strongly that human readable strings are necessary here. I don't think the case has been made for them. At this point, we'll need the implementers to make a case for why these values are human readable and how they plan to internationalize them given your feedback. |
This was not at all the intention behind the change, it was purely to signal that this is intended to be read and consumed by a developer NOT an end user.My intention was something akin to developer digestible debugging tokens. Does anyone have any suggestions for language that implies the audience is humans but it does not need to be natural language? |
@mkhraisha The text you have does that? It's fine to use strings instead of numbers as code-like IDs. This is what, for example, However, we want to make it unappealing to show these (usually English) strings to actual end users outside of a debugging environment. It's a super common I18N bug to assume that the word(s) in the error will mean something to the user (who might not speak English). So the I18N group is "okay" with you having a string there for debugging. And we recognize that developers might abuse that string by putting a displayable value in there--we can't stop them. But your example should use code-like formatting ( |
I have reverted the example changes to be inline with the previous ones, @msporny does that resolve your CR? |
@@ -636,10 +636,10 @@ <h3>BitstringStatusListCredential</h3> | |||
minimum two properties: | |||
<ul> | |||
<li id="status"> | |||
`status`, being a string of the hex value of the status | |||
`status`, being a machine-readable string of the hex value of the status |
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
or 0x1
, but DEADBEEF
, -1e
, or 10EF
are "hex strings" too. Perhaps:
`status`, being a machine-readable string of the hex value of the status | |
`status`, a string representing the hexadecimal value of the status prefixed with `0x` |
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 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 the message
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 that message
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.
@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.
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.
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:
- This is a required feature for Traceability, that's the only place the requirements for this feature are coming from at this moment in time.
- @aphillips seems to agree that if the "message" field is only meant for developers that we don't need a full localization treatment on the field.
- There is no way that we can provide "packed" encoding for the status field unless we use some sort of binary expression for the
status
value.
As such, we're just going to go with all that, merge this PR (with @aphillips additions), and call it a day.
@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 with 0x1.
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.
Co-authored-by: Addison Phillips <[email protected]>
Co-authored-by: Manu Sporny <[email protected]>
Normative, multiple reviews, changes requested made (will apply @aphillips' changes post-merge), no objections, merging. |
Status field should be machine readable, message should be human readable, I changed the example to make the message more clearly human readable.
Preview | Diff