Skip to content

fix: suggest hyphen in array#901

Open
p-spacek wants to merge 8 commits intoredhat-developer:mainfrom
jigx-com:fix/suggest-hyphen-in-arrays
Open

fix: suggest hyphen in array#901
p-spacek wants to merge 8 commits intoredhat-developer:mainfrom
jigx-com:fix/suggest-hyphen-in-arrays

Conversation

@p-spacek
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
this.addSchemaValueCompletions(s.schema.items, separatorAfter, collector, types, 'value', true);
} else {
this.addSchemaValueCompletions(s.schema.items, separatorAfter, collector, types, 'value');
this.addSchemaValueCompletions(s.schema.items, separatorAfter, collector, types, 'value', true);
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

coveralls commented Oct 13, 2023

Coverage Status

coverage: 84.342% (+0.03%) from 84.313%
when pulling 0f1b49a on jigx-com:fix/suggest-hyphen-in-arrays
into 86905c1 on redhat-developer:main.

@gorkem
Copy link
Copy Markdown
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