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

Code editor auto-completion by schema #1435

Merged
merged 24 commits into from
Jun 10, 2022

Conversation

meirotstein
Copy link
Contributor

@meirotstein meirotstein commented Apr 20, 2022

This PR adds the ability to get an auto-completion suggestions based on a given JSON schema for code editor.

It is related to issue #968 .

This feature was implemented by leveraging the ace editor autocomplete capability, and it is mainly about investigating the schema (including its references), and building a set of suggestions that can be delivered to ace live auto-completeion, based on the current cursor position in the JSON document.

  • The feature is currently available only for code mode
  • The feature is disabled by default and can be enabled by setting the boolean allowSchemaSuggestions option to true
  • A prerequisite to this feature is to have a valid schema configured
  • The feature works also with recursive schemas - which are schemas that have a circular references

Limitation :
This feature will work only for a valid JSONs, and no suggestion will show if the JSON is not valid.
So, if one wants to get a list of available properties - he/she sould also provide an empty value followed with a comma (when needed) - as can be seen in the short example movie.

autcomplete_example.mov

@meirotstein meirotstein changed the title Schema autocompletion Code editor auto-completion by schema Apr 21, 2022
@meirotstein meirotstein marked this pull request as ready for review April 21, 2022 13:57
@meirotstein
Copy link
Contributor Author

meirotstein commented Apr 21, 2022

@josdejong - it took a while :-) but now ready for your review. tnx!

@josdejong
Copy link
Owner

Wow 😎 that looks really promising Meir!

I'll review your PR soon.

@josdejong
Copy link
Owner

I've tried out the autocompletion, it works really nicely! The code looks neat too 👍

A couple of feedbacks:

  1. Can you remove the yarn.lock file from the PR?

  2. Currently, the SchemaTextCompleter is configured when creating the editor. I think it should be updated when the schema is changed via the method setSchema.

  3. I like it that the autocompletion functionality is neatly contained in SchemaTextCompleter. Is it possible to write a couple of unit tests to verify that .getCompletions() works (and keeps working with future changes)?

  4. About the limitation you report:

    This feature will work only for a valid JSONs, and no suggestion will show if the JSON is not valid.

    Do you see any possibility to improve on that? It's indeed the normal way I enter new keys and values to just first start typing the key (and needing autocomplete then) and next start typing the value. So it would be a pity if this common use case cannot be supported.

    Some ideas: do a couple of tries to turn invalid JSON into valid JSON to at least figure out the current path. For example: remove the current line and try to parse the JSON, and if needed remove a trailing comma from the previous line and try to parse. Or run jsonrepair and see if that fixes things.

@meirotstein
Copy link
Contributor Author

@josdejong 1-3 - good points. will do it.

As for 4 - I totally agree - this is a significant downside of the current implementation as the use case of adding new property is very common - also this prevents of adding non-string suggesstions in some cases.

However I'm afarid that the solution is a bit complicated than just trying to manipuilate the JSON to be a valid one, because it is also crucial for us to know the exact cursor location in the code editor - this can be influenced by additinal factors such as newlines, tabs, etc. the result of it can be that the completer will 'think' that the cursor is on different location and will provide wrong suggestions - IMO it is worse than no suggestions at all.

Currently I am leveraging the json-source-map lib (which is also in use in the jsoneditor for reporting useful schema errors) which also provides the location for each JSON path - what I have in mind is to somehow integrate with the ace JSON parser, and get the AST which may be able to provide the path and cursor location also for non-valid JSONs.

Another interesting lib I have noted for myself to check is partial-json-parser but still looks like it just emitting the 'bad' parts so I still not sure about it.

In any case, this can be a challenge and require additional research, based on my available time it can take a while and I didnt wanted to delay the entire feature, so my suggestion is to create a new issue for improvmenets and I (or anyone else who is interested) can work on it on my/his/her spare time :-)

WDYT?

@josdejong
Copy link
Owner

Thanks.

Yes makes sense to address point (4) separately. It may involve quite some experimentation. So let's finish this PR without (4) first then 👍

I wasn't aware of partial-json-parser. It looks like a simpler version of jsonrepair that we're using in jsoneditor already (I created it for jsoneditor).

@meirotstein
Copy link
Contributor Author

@josdejong I have addressed the items we discussed, pls review when you can. tnx.

@josdejong
Copy link
Owner

Thanks Meir, I'll review your PR soon, but I'm not sure if I can make it this week.

Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

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

Thanks Meir for the updates. I'm happy with the unit tests, that helps preventing regressions in the future.

I made one small inline remark, besides that everything is ready to merge 👌

src/js/textmode.js Outdated Show resolved Hide resolved
@josdejong josdejong merged commit 93ce059 into josdejong:develop Jun 10, 2022
@josdejong
Copy link
Owner

@meirotstein I just merged your work, thanks again.

Testing once more gives an odd issue on my side: the dropdown is displayed way too low. Does the same happen on your computer?

afbeelding

@meirotstein
Copy link
Contributor Author

Hey @josdejong
I have tested it locally on Chrome and FF and it seems to be OK.
Whats I have discovered is that on Safari the example does not load at all when the autiocomplete option is on. I will look into that.

Can you elaborate on your testing? which browser?

@meirotstein
Copy link
Contributor Author

@josdejong correction: working also in Safari (I have rebuilt it again).

@josdejong
Copy link
Owner

I'm using Windows 11 and tested with the latest Firefox and Chrome, and using Node@16 and npm@8.

I did a bit of debugging:

  1. your branch schema_completion works ok
  2. the current develop branch (where the PR is merged) does not work ok
  3. merging the latest develop into schema_completion does not work
  4. The schema_completion branch is based on v9.7.4. Since then there where only two commits: d9c3220 and d74df9c, which seem unrelated to the new feature of this PR.
  5. I cherry picked the latest commits from develop one by one into schema_completion, and the issue starts occurring as soon as we merge d74df9c (updating dependencies, fixing lint issues)
  6. I was thinking the update of ace-builds could be the cause, but taking the original schema_completion and updating all dependencies there doesn't give the issue.
  7. Then I was realizing that there are new dependency updates since the current develop, so I was comparing apples and pears. Updating all dependencies of develop again solved the issue 🎉. The package.json has an inexact version ^1.5.3, which also matches 1.6.0, that was why I had trouble reproducing the issue consistently. Last time I tested before the weekend this would install 1.5.3 and now it installs 1.6.0. So: schema_completion uses ace-builds@1.4.14, develop has ace-builds@1.5.3, and the latest version of ace is now ace-builds@1.6.0. Looking at the change log of Ace editor, I read a bug fix "Autocomplete stopped working after upgrade to v1.5.2", see https://github.com/ajaxorg/ace/blob/master/CHANGELOG.md#160-2022-06-10. I was hitting that regression and it is solved now in the latest version of Ace 😅 .

@josdejong
Copy link
Owner

I've published v9.9.0 now with the auto-completion feature. Thanks again Meir.

I have made a few minor changes: run the linting and fixed code style accordingly, and increment the numbering of the examples, there was an other example added in the meantime.

@meirotstein
Copy link
Contributor Author

Awesome, thanks!

@meirotstein
Copy link
Contributor Author

@josdejong I have created a new issue to address the non-valid JSON limitaion - #1443

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.

None yet

2 participants