refactor(errors): improve error messages #390
Merged
Conversation
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
stefanblaginov
force-pushed
the
sb-improve-error-messages
branch
from
803ce7f to
80a9674
Compare
jeromesimeon
reviewed
Mar 4, 2022
There was a problem hiding this comment.
Looks good @stefanblaginov !
I've placed a couple of comments for things that should be doublechecked.
Also, I feel there is more work we should be doing on error messages:
- Check that all those errors are actually used, some of them may be obsolete and could be removed
- Check for use of other errors that aren't listed in the
message/en.jsonand use a different approach e.g.,throw new Error("This is some error which is thrown by hand")
So I wouldn't mind leaving your issue open and listing those are additional work that would be useful to do.
|
|
||
| // set model to a Boolean | ||
| cObject.model = true; | ||
| ( function() {serializer.toJSON(cObject);}).should.throw(/.+expected type String/); | ||
| ( function() {serializer.toJSON(cObject);}).should.throw('Model violation in the "org.acme.Vehicle#CAR_123" instance. The field "model" has a value of "true" (type of value: "boolean"). Expected type of value: "String".'); |
There was a problem hiding this comment.
Am I the only one finding those changes confusing, surely "1" is a String!
| @@ -233,7 +233,7 @@ describe('ObjectValidator', function () { | |||
|
|
|||
| (function () { | |||
| objectValidator.visit(concerto.getTypeDeclaration(data), parameters); | |||
| }).should.throw(/Instance undefined has property previousOwners with type undefined that is not derived from test.Person\[\]/); | |||
| }).should.throw('Instance "undefined" has a property "previousOwners" with type "undefined" that is not derived from "test.Person[]".'); | |||
There was a problem hiding this comment.
I'm wondering if the undefined here is a bug? the sentence doesn't read quite right. Could we double check?
| @@ -264,7 +264,7 @@ describe('ObjectValidator', function () { | |||
| parameters.stack = new TypedStack(data); | |||
| (function () { | |||
| objectValidator.visit(concerto.getTypeDeclaration(data), parameters); | |||
| }).should.throw(/expected a Relationship/); | |||
| }).should.throw('Model violation in the "undefined" instance. Class "test.Person" has a value of "[object Object]". Expected a "Relationship".'); | |||
There was a problem hiding this comment.
I'm wondering if the undefined here is a bug? the sentence doesn't read quite right. Could we double check?
Signed-off-by: Stefan Blaginov <[email protected]>
stefanblaginov
force-pushed
the
sb-improve-error-messages
branch
from
80a9674 to
f45cadc
Compare
Signed-off-by: Stefan Blaginov <[email protected]>
stefanblaginov
force-pushed
the
sb-improve-error-messages
branch
from
3e0c82e to
219cbf6
Compare
mttrbrts
approved these changes
Apr 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #389
Changes
https://ux.stackexchange.com/questions/18671/periods-at-the-end-of-a-sentence-in-alert-message
https://stackoverflow.com/questions/1136829/do-you-end-your-exception-messages-with-a-period
Flags
Code dependant on parsing
concerto-coreerror messages is likely to break from this change.Author Checklist
--signoffoption of git commit.masterfromfork:branchname