jsonnext / codemirror-json-schema

A JSONSchema enabled mode for codemirror 6, for json4 and json5, inspired by monaco-json
https://codemirror-json-schema.netlify.app
MIT License
57 stars 9 forks source link

json5 insert text, prefer defaults #48

Closed acao closed 11 months ago

acao commented 11 months ago

this allows us to:

I tested this with packageJson.types, which is the only value in the schema that has a default

changeset-bot[bot] commented 11 months ago

⚠️ No Changeset found

Latest commit: a138f764f5ac08627ac2c18f459bd58eb500570d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

netlify[bot] commented 11 months ago

Deploy Preview for codemirror-json-schema ready!

Name Link
Latest commit a138f764f5ac08627ac2c18f459bd58eb500570d
Latest deploy log https://app.netlify.com/sites/codemirror-json-schema/deploys/64cf531b167c350008597d4c
Deploy Preview https://deploy-preview-48--codemirror-json-schema.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

acao commented 11 months ago

@imolorhe so now we are at a much more comprehensive level of coverage, but I would like this to be 95%+ before making this change

image

Here are several regressions I noticed, I will check to see if they are on main branch as well: json4-regression

json4-regression-2

as you can see, with double quotes it's fine, but I think something related to the modified json5 logic has it working wrong json4-fine-double

acao commented 11 months ago

@imolorhe i see you approved this but I have noted a few regressions, I'm not sure if they came before or after this PR! Should I just merge and we can work out the bugs after that?

imolorhe commented 11 months ago

oh we can fix it then

acao commented 11 months ago

ok, confirmed with tests that the regression is not caused by this PR. we can go ahead and merge this, and check out #54 to fix this bug