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

fix: suggest hyphen in array #901

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

p-spacek
Copy link
Contributor

@p-spacek p-spacek commented Jul 11, 2023

What does this PR do?

  1. - (array item) suggestion was missing for some specific situations, for example: enums.
    it worked ok for 1st item after a colon, but not for 2nd

after fix:
image

UPDATE
I extended this PR because there were related issues, with a tiny update of this original problem, I can fix another 2 problems:
2) auto add - when hyphen is missing
image
choose item will produce incorrect yaml
image

  1. The default snippet with a simple string will produce this weird result when the string value is defined in snippet body
image

What issues does this PR fix or reference?

no ref

Is it tested? How?

modified existing
added one test for the enum

  • check for - array items
  • check for -
    added test for default snippets with string value in the body property

@p-spacek p-spacek force-pushed the fix/suggest-hyphen-in-arrays branch from 46c0d3c to 2af180a Compare July 11, 2023 10:24
result.items.map((i) => ({ label: i.label, insertText: i.insertText })),
[
{
insertText: 'Test',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there should be '- Test' insert text, but it's another problem

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you file that as an issue?

Copy link
Contributor Author

@p-spacek p-spacek Oct 11, 2023

Choose a reason for hiding this comment

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

I fixed it in this PR the end...

@p-spacek p-spacek force-pushed the fix/suggest-hyphen-in-arrays branch from 2af180a to 519f984 Compare September 4, 2023 12:53
} else {
this.addSchemaValueCompletions(s.schema.items, separatorAfter, collector, types, 'value');
this.addSchemaValueCompletions(s.schema.items, separatorAfter, collector, types, 'value', true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unify the code for both scenarios. If a schema contains items it's an array. No need to check if type === 'object'

@p-spacek
Copy link
Contributor Author

p-spacek commented Sep 5, 2023

Hello @msivasubramaniaan, can I ask you for a review?

gorkem
gorkem previously approved these changes Oct 9, 2023
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.

LGTM.

Needs to be rebased before I can merge and the failing CI (which looks like a GH problem) should clear with the rebase.

Another level of improvement to this PR would be to filter the items that are already on the array to avoid duplicate entries on the array. Approving this PR but please let me know if you are planning to add the improvement.

@p-spacek p-spacek force-pushed the fix/suggest-hyphen-in-arrays branch from d0872b1 to 86a72ed Compare October 11, 2023 10:33
…-yaml-language-server into fix/suggest-hyphen-in-arrays
@p-spacek
Copy link
Contributor Author

@gorkem , I don't think that there should be such a filter/condition
because schema

{
          type: 'object',
          properties: {
            references: {
              type: 'array',
              items: {
                enum: ['Test', 'Test2'],
              },
            },
}

doesn't define that items of the array have to be unique. To be honest I am not sure how to define this (unique array items)...

This should be allowed based on the schema

items:
 - Test1
 - Test1
 - Test2

or don't you agree? did I understand you incorrectly?

@p-spacek
Copy link
Contributor Author

I found another related problem
So I converted this PR to a draft
I'll try to fix it at once

@coveralls
Copy link

Coverage Status

coverage: 83.841% (+0.03%) from 83.813% when pulling 8e4d05f on jigx-com:fix/suggest-hyphen-in-arrays into ed03cbf on redhat-developer:main.

@gorkem
Copy link
Collaborator

gorkem commented Oct 13, 2023

Correct, item uniqueness is determined by the schema. The current implementation does not take into account the uniqueItems flag. An improvement to the current implementation would be checking this flag and filtering accordingly. In general, code assist should not suggest values leading to syntax errors, even though it is a trivial fix.

@gorkem gorkem removed their request for review October 15, 2024 20:22
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.

3 participants