- 
                Notifications
    You must be signed in to change notification settings 
- Fork 35
Add 22NBE and 22NBF #374
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 22NBE and 22NBF #374
Conversation
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.
Thanks, @sherfert. I've added some suggestions in case it is still possible to change the messages in the code base.
| = 22NBE | ||
|  | ||
| == Status description | ||
| error: data exception - invalid vector dimensions. Invalid vector dimensions. The number of vector dimensions needs to be between `{ <<count>> }` and `{ $count1 }`, but was `{ $count2 }`. | 
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.
Is it possible to change it in the codebase to:
| error: data exception - invalid vector dimensions. Invalid vector dimensions. The number of vector dimensions needs to be between `{ <<count>> }` and `{ $count1 }`, but was `{ $count2 }`. | |
| error: data exception - invalid vector dimensions. Invalid vector dimensions. The number of vector dimensions must be between `{ <<count>>1 }` and `{ $count2 }`, but is `{ $count3 }`. | 
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.
Sure, I can change 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.
The arguments are called count, count1 and count2 though. Not count1, count2 and count3. That is in line with other numbered arguments we use.
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.
Could you give me an example? To me, count sounds like something in general, not a specific number,  while count 1, 2, 3, etc. are specific numbers.
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.
At least, in the error messages, when the same variable is mentioned more than once in a message, we add 1, 2, 3, etc., to show that they have different values.
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.
At least, in the error messages, when the same variable is mentioned more than once in a message, we add 1, 2, 3, etc., to show that they have different values.
My bad, you're absolutely right. I'll fix that (here and in the monorepo, too).
| = 22NBF | ||
|  | ||
| == Status description | ||
| error: data exception - property value too big. Property value of type `{ <<typeDescription>> }` too big (more than `{ <<bytes>> }` bytes): `{ <<value>> }` | 
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.
same here:
| error: data exception - property value too big. Property value of type `{ <<typeDescription>> }` too big (more than `{ <<bytes>> }` bytes): `{ <<value>> }` | |
| error: data exception - property value too big. Property value of type `{ <<typeDescription>> }` is too big (more than `{ <<bytes>> }` bytes): `{ <<value>> }`. | 
What would be the actual message that the users will receive? We try not to use the old formatting: message: what was wrong, but instead follow the guidelines: problem+cause+solution (if possible). That would mean the message would look something like this:
error: data exception - property value too big. The property value `{ <<value>> }` of type `{ <<typeDescription>> }` is too big (more than `{ <<bytes>> }` bytes).
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.
Adding "is" is no problem. I would rather not move { <<value>> }, though. It will without exception be very big (100 characters), since this error is only thrown for too big values. I think the error message is more readable if that big blob of data comes at the end, wdyt?
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.
Fair enough.
| Thanks for the documentation updates. The preview documentation has now been torn down - reopening this PR will republish it. | 
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.
Thank you, @sherfert. Looks good to me now.
| 
 Thanks! | 
No description provided.