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

Better readable and consistent path style #815

Closed
radicarl opened this issue Nov 26, 2019 · 8 comments
Closed

Better readable and consistent path style #815

radicarl opened this issue Nov 26, 2019 · 8 comments
Assignees

Comments

@radicarl
Copy link

User story.
I would like to have better readable JSON/YAML-path in the output of the linter.
The wildcard {{path}} uses / for element separation and ~1 for / in element names. This is not very readable. I think . as a separator is in YAML and JSON more commen than the / and in this case the slashes in element names could be slashes.

paths./my/resource/{id}/nested-resource.get.responses.200.content.application/json
# better than
paths/~1my~1resource~1{id}~1nested-resource/get/responses/200/content.application~1json

I also noticed at least one different representation:

oas3-schema          /paths//my/resources/get/responses/200 should have required property '$ref'

Describe the solution you'd like
Path representation in JSON-Path-like-Notation. The leading $. could help to identify the path in the output message.

@radicarl radicarl changed the title Better and consistent path style Better readable and consistent path style Nov 26, 2019
@P0lip P0lip self-assigned this Dec 5, 2019
@P0lip P0lip mentioned this issue Dec 12, 2019
4 tasks
@P0lip
Copy link
Contributor

P0lip commented Dec 14, 2019

@radicarl I investigated a couple of possibilities, and I believe it'd ideal to let user decide.
I could expose an option, like pathStyle. What do you think?

The inconsistency in path is an issue regardless though, and something I'm addressing right now.

@P0lip
Copy link
Contributor

P0lip commented Dec 15, 2019

One more issue I stumbled upon when working on #839.
I'm leaning towards displaying the parts of paths when a given property is required, but does not exist in the document.
Displaying lengthy paths is not really helpful and makes the messages less readable.
That means, if $.foo.bar exists in the document, but the type is wrong, we'd just display "should be string" message, but if $.foo.bar would not exist in the document, "bar should be a string" would be printed instead.

@radicarl
Copy link
Author

@radicarl I investigated a couple of possibilities, and I believe it'd ideal to let user decide.
I could expose an option, like pathStyle. What do you think?

The inconsistency in path is an issue regardless though, and something I'm addressing right now.
An option would be great. Thanks.

One more issue I stumbled upon when working on #839.
I'm leaning towards displaying the parts of paths when a given property is required, but does not exist in the document.
Sorry, but this I don't understand.

Displaying lengthy paths is not really helpful and makes the messages less readable.
That means, if $.foo.bar exists in the document, but the type is wrong, we'd just display "should be string" message, but if $.foo.bar would not exist in the document, "bar should be a string" would be printed instead.
The complete path in the message gives me a lot of context infomation and helps me to evaluate the output of the linter, without looking in the linted document first.
Actually, my wrapper scripts scans the message for the appearance of the path and adds the path to the message if it not exists.

@radicarl
Copy link
Author

radicarl commented Dec 16, 2019

@radicarl I investigated a couple of possibilities, and I believe it'd ideal to let user decide.
I could expose an option, like pathStyle. What do you think?

The inconsistency in path is an issue regardless though, and something I'm addressing right now.

An option would be great. Thanks.

One more issue I stumbled upon when working on #839.
I'm leaning towards displaying the parts of paths when a given property is required, but does not exist in the document.

Sorry, but this I don't understand.

Displaying lengthy paths is not really helpful and makes the messages less readable.
That means, if $.foo.bar exists in the document, but the type is wrong, we'd just display "should be string" message, but if $.foo.bar would not exist in the document, "bar should be a string" would be printed instead.

The complete path in the message gives me a lot of context infomation and helps me to evaluate the output of the linter, without looking in the linted document first.
Actually, my wrapper script scans the message for the appearance of the path
and adds the path to the message if it not exists.

@P0lip
Copy link
Contributor

P0lip commented Dec 16, 2019

One more issue I stumbled upon when working on #839.
I'm leaning towards displaying the parts of paths when a given property is required, but does not exist in the document.

Actually, this is a short intro to

Displaying lengthy paths is not really helpful and makes the messages less readable.
That means, if $.foo.bar exists in the document, but the type is wrong, we'd just display "should be string" message, but if $.foo.bar would not exist in the document, "bar should be a string" would be printed instead.

The complete path in the message gives me a lot of context infomation and helps me to evaluate the output of the linter, without looking in the linted document first.
Actually, my wrapper script scans the message for the appearance of the path
and adds the path to the message if it not exists.

I understand, there might be different use cases. One may want to see the whole path in the message, the others just the missing part.
I don't have any strong preference, but we do need to be consistent regardless of the choice we go with.
Right now, we sometimes report the whole path, sometimes just the missing part, and sometimes something else. This is not a huge issue, but being consistent will provide a nicer UX.

@philsturgeon @marbemac
What do you think?
I see 3 options:

  • short paths only (displaying piece of paths that do not exist in the document)
  • full paths (displaying the whole path)
  • supporting both via an option

@radicarl
Copy link
Author

I would prefer an option. For consistency the {{path}}-placeholder in own rules should be affected from the option or your choice too.
If you decide to only support short paths in messages, could you add the short path as field of the json output? This way I could replace the short paths with the long paths.

@P0lip
Copy link
Contributor

P0lip commented Dec 17, 2019

If you decide to only support short paths in messages, could you add the short path as field of the json output? This way I could replace the short paths with the long paths.

I believe an option might be the only choice, as built-in rules will shortly start using {{path}} as well, therefore keeping the message consistent may be troublesome.

@P0lip P0lip mentioned this issue Dec 17, 2019
4 tasks
@philsturgeon
Copy link
Contributor

I agree that a we could make our messages clearer by not using path as much as we do.

Apart from the $ref error thing (which is another issue sadly) I think we should be using the message for explaining to a human what is going on, and i think that path is not always doing that. We should have the path available for people to programmatically use in the JS API, and maybe we can display it in a tooltip or something, but we are already giving them a line number and taking them to the place with the error on click, so maybe displaying the target is more relevant.

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

No branches or pull requests

3 participants