Skip to content

Show schema errors with unmapped locations in ace editor mode (#556)#606

Closed
daggaz wants to merge 1 commit into
josdejong:developfrom
daggaz:master
Closed

Show schema errors with unmapped locations in ace editor mode (#556)#606
daggaz wants to merge 1 commit into
josdejong:developfrom
daggaz:master

Conversation

@daggaz

@daggaz daggaz commented Dec 3, 2018

Copy link
Copy Markdown

This PR changes the display of schema/custom errors in "code" view.

Any errors that do not have a valid path are now shown in the jsoneditor-text-errors table at the bottom of the editor area.

@josdejong

Copy link
Copy Markdown
Owner

Thanks for your PR @daggaz! Do you have an example of a JSON document and schema were you get these unmapped locations? (so I can easily test your PR?)

@meirotstein

Copy link
Copy Markdown
Contributor

@daggaz @josdejong
Errors with no path (given from ajv) are mapped to the top of the document. For example;
img_20181203_211541

You can try it on the following editor by misspelling the word "image" (for instance):
https://livepersoninc.github.io/json-pollock/editor/

@daggaz

daggaz commented Dec 3, 2018

Copy link
Copy Markdown
Author

It's not errors with no path (i.e. .), it's errors with a bad path (a path not in the document). Those errors would be shown in text view, but are silently discarded in code view.

It's only really possible if you have a custom validation function that behaves badly. It's also super useful for when you are trying to debug said function...

@meirotstein

Copy link
Copy Markdown
Contributor

@daggaz got it. this kind of errors indeed not taken into consideration at the moment.
IMHO, if you choose to show this errors it will be better to preserve the current behavior and to show it at the top of the screen instead of adding the errors tables in addition to the annotations.

@josdejong

Copy link
Copy Markdown
Owner

I like your idea @meirotstein , adding these kind of errors to the top level errors. What do you think @daggaz ?

@josdejong

Copy link
Copy Markdown
Owner

ping @daggaz

@daggaz

daggaz commented Dec 23, 2018

Copy link
Copy Markdown
Author

I kinda like that the un-mappable errors are come up very obviously, but I don't have strong feelings either way.

The patch we have works, so I vote just do that.

@josdejong

Copy link
Copy Markdown
Owner

The patch we have works, so I vote just do tha

@daggaz so you mean you would like to keep this PR as it is right now?

I'm in favor of not silently hiding errors that don't have a (correct) path, but display them at root node, so at least the user knows that something is wrong. So that would require a change in the PR. What do you think?

@daggaz

daggaz commented Dec 30, 2018

Copy link
Copy Markdown
Author

Like I said, personally I like that those errors come up in a more obvious/jarring way, but I'm willing to acquiesce.

@meirotstein

Copy link
Copy Markdown
Contributor

I'm working on a PR of showing the errors table in code mode in addition to text mode with the option of hiding it - both fixes can provide a quite good solution for this kind of errors.

@josdejong

Copy link
Copy Markdown
Owner

Sounds good @meirotstein ! I will check out your PR asap but have to do a bit of catch-up after the holidays.

@daggaz would that address your issue?

@daggaz

daggaz commented Jan 3, 2019

Copy link
Copy Markdown
Author

I'm happy

@josdejong

Copy link
Copy Markdown
Owner

Great. I've merged the improvement of @meirotstein, will close this PR now.

@josdejong josdejong closed this Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants