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: show examples on hover. #660

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

yassun7010
Copy link
Contributor

What does this PR do?

If "examples" is defined in the schema, display it on the hover.

I think it useful to show what kind of input is expected for a string in user format.

Is it tested? How?

A test was added.

@evidolob
Copy link
Collaborator

@yassun4dev We have test failing on this PR.
Also could you fix sign of the commit, it seems that you use two accounts.

@yassun7010
Copy link
Contributor Author

yassun7010 commented Feb 14, 2022

@evidolob Oh, sorry...

I fixed sign of the commit, and check why the test failed on Linux only.
Try again CI, the pipeline was successful.

@coveralls
Copy link

coveralls commented Feb 14, 2022

Coverage Status

Coverage increased (+0.2%) to 81.356% when pulling a58c82f on yassun4dev:add_examples_hover into 2f34acf on redhat-developer:main.

@gorkem
Copy link
Collaborator

gorkem commented Feb 14, 2022

@yassun4dev Are there any JSON schemas that you can suggest for testing this functionality?

@yassun7010
Copy link
Contributor Author

@gorkem I added one test case and put a simple schema in it.

You need more comprehensive testing as separate schema files?

@gorkem
Copy link
Collaborator

gorkem commented Feb 16, 2022

Not necessarily for testing but wanted to see real world usages.

@yassun7010
Copy link
Contributor Author

@gorkem I see.

I have encountered some cases that require examples in real world.

  1. It is too complicated to describe with 'pattern', and 'examples' assists the reading.
{
  "type": "object",
  "properties": {
    "range": {
      "type": "string",
      "description": "Sheet range of Excel.",
      "pattern": "^[A-Z]+[1-9][0-9]*:[A-Z]+[1-9][0-9]*$", // It's too complicated pattern.
      "examples": ["A1:B12"] // Easy to read and understand. 
    }
  }
}
  1. Since the exception data already exists, "enum" and "pattern" cannot be used, but a recommended hint is needed.

Copy link
Collaborator

@gorkem gorkem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. Just one comment on the format of the hover. It is up-to you if you want to address it or not.

if (result.length > 0) {
result += '\n\n';
}
result += 'Examples: [\n\n';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big one but I am not sure if we need the [ in here. The hovers looked better for me without them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gorkem I fixed hover format. I think it looks better. Thank you!!

@evidolob evidolob merged commit b889fbe into redhat-developer:main Feb 23, 2022
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.

5 participants