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 required attributes insertion with first array item #272

Merged

Conversation

apupier
Copy link
Member

@apupier apupier commented Jul 8, 2020

it seems that a \t is used instead of 4 spaces. I thought that tabs shouldn't be used in yaml files. To investigate more. @JPinkney does it ring a bell to you?

  1) yamlCompletion with required properties
       doComplete
         Insert required attributes at correct level:

      AssertionError [ERR_ASSERTION]: 'top:\n\tprop1: $1' == 'top\n    prop1: $1'
      + expected - actual

      -top:
      -	prop1: $1
      +top
      +    prop1: $1

@JPinkney
Copy link
Contributor

JPinkney commented Jul 8, 2020

I can't recall \t being used anywhere but I can a look

@apupier
Copy link
Member Author

apupier commented Jul 8, 2020

there are 12 places where the \t is used in yamlCompletion.ts.
for instance

insertText: this.getInsertTextForProperty(key, propertySchema, addValue, separatorAfter, identCompensation + '\t'),

@JPinkney
Copy link
Contributor

JPinkney commented Jul 8, 2020

Yep, just saw those. Those will need to be switched to use whatever the user has set has their number of spaces in the document

@apupier
Copy link
Member Author

apupier commented Jul 8, 2020

in this case, isn't the problem that it should be 2 tabs instead of a single one? I think the "array" level is not counted in this specific case.

Based on specification, each level can have a different number of spaces https://yaml.org/spec/1.2/spec.html#id2777534 ( :'-( ) will be funny to search for number of spaces on other elements on the same indentation level, when there is one. If not, which defaults? 2 spaces par indentation level?

@apupier apupier force-pushed the requiredAttributesWrongIndentation branch from fdbe820 to c3254a7 Compare July 8, 2020 13:44
redhat-developer/vscode-yaml#312

it occurs only with top-level element

Signed-off-by: Aurélien Pupier <[email protected]>
@apupier apupier force-pushed the requiredAttributesWrongIndentation branch from c3254a7 to e380abf Compare July 8, 2020 13:47
@apupier
Copy link
Member Author

apupier commented Jul 8, 2020

the problem occurs only when it is the first element in the document. I updated the PR to provide a a fix and test the 2 cases.

@JPinkney I only changed a > to a >=. Tests are passing. Do you remind if using a strict equality was useful in a specific case?

for usage of tabs, i think this would be nice to handle too but in another tasks (which seems not trivial)

@apupier apupier changed the title [Work In Progress]Test for completion of required attributes Test for completion of required attributes Jul 8, 2020
@apupier apupier requested a review from JPinkney July 8, 2020 13:51
@apupier apupier changed the title Test for completion of required attributes Fix required attributes insertion with first array item Jul 8, 2020
@JPinkney
Copy link
Contributor

JPinkney commented Jul 8, 2020

will be funny to search for number of spaces on other elements on the same indentation level, when there is one

I was thinking more that if we could find a way to detect what number of spaces the editor was set to use then we could use that, otherwise default to 2. I know that in the lsp tabSize is available for formatting but i'm not sure we can grab that somewhere else and then use that to insert instead of \t but yeah I think this can be done in another PR

Do you remind if using a strict equality was useful in a specific case?

I don't remember off the top of my head, that code was added a long time ago in this PR:
ad07244. Since all of the autocompletion5 tests are passing it seems like there isn't a regression anywhere

@JPinkney JPinkney merged commit dcbf850 into redhat-developer:master Jul 10, 2020
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.

2 participants