Skip to content

Add a more sensible fallback range if there is no parent property#30

Closed
Levertion wants to merge 1 commit intomicrosoft:masterfrom
Levertion:fallback-range
Closed

Add a more sensible fallback range if there is no parent property#30
Levertion wants to merge 1 commit intomicrosoft:masterfrom
Levertion:fallback-range

Conversation

@Levertion
Copy link
Contributor

@Levertion Levertion commented Oct 24, 2018 •
edited
Loading

EDIT: After some consideration I realise this should be an issue rather than a pull request, as this is not obviously better in all cases.

Before:

{"bold":true}
^
Missing property "text"

After:

{"bold":true}
^^^^^^^^^^^^^
Missing property "text".

This case could occur if the entire document was validated against a schema using `required` or `oneOf`. This would have the disadvantage of making the errors uglier in cases where there is a large document which fails this validation, however only erroring the first character is also not great. Perhaps we should work out an alternative method?

@aeschli
Copy link
Collaborator

aeschli commented Oct 24, 2018

Multi line squiggles or even the whole document being red is really ugly and that was the reason to make the error that small.
I'm not really in flavor of this change.

@Levertion
Copy link
Contributor Author

Levertion commented Oct 24, 2018

Yeah, that's entirely reasonable. My use-case is for another format with inline json support on one line, where the single space squiggle appears to be too small, but it becomes ugly for a larger string anyway.

There is no good solution, but I'll close this.

@Levertion Levertion closed this Oct 24, 2018
This was referenced Oct 24, 2018
Merged
Merged
@Levertion
Copy link
Contributor Author

Levertion commented Nov 5, 2018

Would it be worth having an heurstic such as if the node length is less than, say 35 we give an error over the whole range, else give an error on the starting character - for smallish sections the small error range is somewhat unfortunate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@aeschli aeschli

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Levertion @aeschli