json5 insert text, prefer defaults #48
Conversation
|
✅ Deploy Preview for codemirror-json-schema ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/json-completion.ts
Outdated
| } | ||
| } | ||
| if (!value || nValueProposals > 1) { | ||
| if (!value || nValueProposals < 1) { |
There was a problem hiding this comment.
I wasn't sure what the intention was here, so I reversed the logic
in the case of packageJson.types we have two value proposals I think - one from default, and another because it was an enum
There was a problem hiding this comment.
I think the idea is that if there are multiple possible values, we don't assume a value for the user
There was a problem hiding this comment.
I think in this case, a schema has both default and enum. in my opinion, we should just guess with default and forego the other snippet guessing logic then, instead of counting? what do you think?
There was a problem hiding this comment.
@imolorhe I have pushed another commit here where i reverted to the original logic for more than 1 proposal, but made it so that if default is provided, only the default value is proposed, how does that sound?
| } | ||
|
|
||
| const currentWord = getWord(ctx.state.doc, node); | ||
| const rawWord = getWord(ctx.state.doc, node, false); |
There was a problem hiding this comment.
this logic is only needed for json5 really, so we can guess how to insert the completion text based on quotation types or lack thereof
| } | ||
|
|
||
| type JSONCompletionOptions = { | ||
| mode?: "json" | "json5"; |
There was a problem hiding this comment.
unfortunately, CompletionContext nor EditorState give us metadata about which parser has parsed the editor state, so we must rely on configuration
acao
force-pushed
the
json5-inserttext-defaults
branch
from
1c590e5 to
6b17074
Compare
| propertySchema.examples[0], | ||
| "" | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
The tests will make us more confident with these type of changes!
There was a problem hiding this comment.
indeed! i will add tests for all of these cases before merging this PR
acao
force-pushed
the
json5-inserttext-defaults
branch
from
04a569e to
039344c
Compare
|
@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
Here are several regressions I noticed, I will check to see if they are on main branch as well:
as you can see, with double quotes it's fine, but I think something related to the modified json5 logic has it working wrong |
acao
force-pushed
the
json5-inserttext-defaults
branch
from
039344c to
4ac9268
Compare
|
@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? |
|
oh we can fix it then |
|
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 |
this allows us to: - automatically complete the json5 string based on the type of quotes or lack thereof that the user is using - prefers the schema.default over other guesses - fixes an apparent bug where guessed solutions are overwritten I tested this with `packageJson.types`, which is the only value in the schema that has a default
this allows us to:
I tested this with
packageJson.types, which is the only value in the schema that has a default