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: array item with existing property #882

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

Conversation

p-spacek
Copy link
Contributor

@p-spacek p-spacek commented May 17, 2023

What does this PR do?

fix autocompletion when the position is on the same line with the hyphen followed by other props

there are two problems:

  1. intellisense doesn't work correctly for the first item when there is another prop in the array item
Screenshot 2023-05-17 at 09 37 56 Screenshot 2023-05-17 at 09 37 24
Screen.Recording.2023-05-17.at.09.38.41.mov
  1. the compensation of the indentation is not correct for the first item after the hyphen
Screen.Recording.2023-05-17.at.09.39.39.mov

after fix of the 1st problem, the 2nd problem looks like this

Screen.Recording.2023-05-17.at.09.57.23.mov

after fix:

Screen.Recording.2023-05-17.at.09.40.47.mov

UPDATE:
3) this PR also fixes this issue:

Screen.Recording.2023-07-17.at.13.04.06.mov
  1. correct indent when the cursor is just after the hyphen with trailing spaces
Screen.Recording.2023-11-29.at.13.54.43.mov

What issues does this PR fix or reference?

no reference

Is it tested? How?

add unit tests

@coveralls
Copy link

coveralls commented May 17, 2023

Coverage Status

coverage: 83.982% (+0.2%) from 83.813%
when pulling 5594fea on jigx-com:fix/array-item-with-existing-property
into ed03cbf on redhat-developer:main.

test/autoCompletionFix.test.ts Outdated Show resolved Hide resolved
test/autoCompletionFix.test.ts Outdated Show resolved Hide resolved
test/autoCompletionFix.test.ts Outdated Show resolved Hide resolved
@@ -144,11 +144,11 @@ export class YamlCompletion {

this.arrayPrefixIndentation = '';
let overwriteRange: Range = null;
const isOnlyHyphen = lineContent.match(/^\s*(-)\s*$/);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if doing this with RegEx is a good idea. For instance this fails on a line like - # a comment. Can we use the AST to check if the rest of the line does not have any semantically significant tokens?

Copy link
Contributor Author

@p-spacek p-spacek Jun 6, 2023

Choose a reason for hiding this comment

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

it was there from the previous implementation, I just reused it (moved it up). I didn't want to change it.

But agree,
I went a little bit deeper and there is another problem.

The real problem is in getNodeFromPosition
This method doesn't give proper results for arrays and maybe some others. That is the reason why there has to be custom code to 'fix' proper node based on special conditions 'empty lines, arrays, ...' (lines 343 to 515 in yamlCompletion)

for example, this yaml return nodes:

foo:
  - # foo object is returned (should be foo[0])
    # foo object is returned (should be foo[0])
    item1: aaaf
    # foo[0] object is returned (OK)
  - # foo object is returned (should be foo[1])
    # foo[!!0!!] object is returned (should be foo[1])
    item2: bbb
    # foo[1] object is returned (OK)

So this PR doesn't make sense anymore (it's possible to reuse tests)

Does your team have the capacity to fix it?
If not I will try it in the future, but don't have much time to do it :-/

@p-spacek p-spacek requested a review from gorkem June 12, 2023 07:22
@p-spacek p-spacek force-pushed the fix/array-item-with-existing-property branch from 856cb85 to 3b3dd25 Compare July 14, 2023 11:49
@p-spacek
Copy link
Contributor Author

@gorkem can you please consider this PR

it will also fix another issue (see updated description 3) )

I think that getNodeFromPosition doesn't work correctly with arrays (check the previous comment) so it's not possible to detect an array by AST (as you suggested),

but this PR doesn't try to reimplement it, this PR fixes a few issues that are caused by a combination of:

  • how the node is selected (getNodeFromPosition)
  • how is the indent array calculated

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