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

Add user-friendly explanations to the JSON compatibility checking #2160

Closed
jsenko opened this issue Jan 10, 2022 · 4 comments · Fixed by #2227
Closed

Add user-friendly explanations to the JSON compatibility checking #2160

jsenko opened this issue Jan 10, 2022 · 4 comments · Fixed by #2227
Labels
Beginner Friendly Solving this issue can help people who are starting with the project learn Documentation Improvements or additions to documentation Quality It's not a bug, but negatively affects user experience or may cause problems in the future Seeking Contribution Suitable for developers who would like to contribute to the project

Comments

@jsenko
Copy link
Member

jsenko commented Jan 10, 2022

Consider this comment #2135 (comment) . The output of the compatibility checker is as follows:

Difference(diffType=OBJECT_TYPE_PROPERTY_SCHEMAS_NARROWED, pathOriginal=, pathUpdated=/propertySchemasAdded, subSchemaOriginal=true, subSchemaUpdated=[NumberSchemaWrapper(wrapped={"type":"number"})])

While this may be helpful to find where the issue is, it does not explain well why the schemas are not compatible. Adding a user-friendly explanation would improve usability and would serve as a documentation for future changes in the library.

@jsenko jsenko added Documentation Improvements or additions to documentation Quality It's not a bug, but negatively affects user experience or may cause problems in the future Beginner Friendly Solving this issue can help people who are starting with the project learn Seeking Contribution Suitable for developers who would like to contribute to the project labels Jan 10, 2022
@famarting
Copy link
Contributor

have into account the usage of the existing objects we have (RuleViolationError and a list of RuleViolationCause, with description and context). IMO it's important to do not change that object and try to make use of it as is now, that way our existing UI integration will continue working properly.

@jhughes24816
Copy link
Contributor

I have been playing around with this a bit today, just starting to find my way around. Would this a be a suitable error message?
RuleViolationException: Incompatible artifact: 3d76a98a-26d7-4afc-a2ba-60cf80a19b72 [JSON], num of incompatible diffs: {1}, list of diff types: [NUMBER_TYPE_MINIMUM_INCREASED/properties/age/minimum]

@jsenko
Copy link
Member Author

jsenko commented Feb 1, 2022

Hi, I would separate the diff type a bit from the path, i.e. NUMBER_TYPE_MINIMUM_INCREASED at /properties/age/minimum, but the error message looks good! Are you interested in contributing?

@jhughes24816
Copy link
Contributor

Hi, Good idea to split it. I'll put a PR in later this morning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Beginner Friendly Solving this issue can help people who are starting with the project learn Documentation Improvements or additions to documentation Quality It's not a bug, but negatively affects user experience or may cause problems in the future Seeking Contribution Suitable for developers who would like to contribute to the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants