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: extra space after position #732

Conversation

p-spacek
Copy link
Contributor

@p-spacek p-spacek commented Jun 9, 2022

What does this PR do?

fix another situation with extra space after the current cursor.
My previous fixes covered only some of the possible situations (#696, #697)

there were still problems in this type of yaml (2nd position)

array:
  - prop1: val1
     prop2: #__ (#cursor, _space)
obj:
  prop1: val1
  prop2: #__ (#cursor, _space)

This fix will solve this situation (for object, arrays in one place) by finding the correct node inside getNodeFromPosition, instead of fixing it later and trying to find a better node (null node).

Another part of this PR is to overwrite range - remove extra space

What issues does this PR fix or reference?

no ref

Is it tested? How?

modified and added unit tests

@coveralls
Copy link

coveralls commented Jun 9, 2022

Coverage Status

Coverage decreased (-0.02%) to 82.595% when pulling b9555f4 on jigx-com:fix/extra-space-after-position into 4956427 on redhat-developer:main.

@@ -115,7 +119,13 @@ export class SingleYAMLDocument extends JSONDocument {
return;
}

if (range[0] <= positionOffset && range[1] >= positionOffset) {
const isNullNodeOnTheLine = (): boolean =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

inside the function because the optimization


this.arrayPrefixIndentation = '';
let overwriteRange: Range = null;
if (node && isScalar(node) && node.value === 'null') {
if (areOnlySpacesAfterPosition) {
overwriteRange = Range.create(position, Position.create(position.line, lineContent.length));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

'remove' extra space after cursor

if (node) {
// when the value is null but the cursor is between prop name and null value (cursor is not at the end of the line)
if (isMap(node) && node.items.length && isPair(node.items[0])) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the mistake was to take only 0. index.
null node could be anywhere inside the collection. So there would have to be some complicated implementation to find the closest null node.

Same problem inside array fix

@@ -1306,7 +1306,7 @@ describe('Auto Completion Tests', () => {
assert.equal(result.items.length, 1);
assert.deepEqual(
result.items[0],
createExpectedCompletion('- (array item)', '- name: ${1:test}', 3, 4, 3, 4, 9, 2, {
createExpectedCompletion('- (array item)', '- name: ${1:test}', 3, 4, 3, 5, 9, 2, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

4->5 because extra space will be removed after inserting the suggested item

@p-spacek p-spacek marked this pull request as ready for review June 9, 2022 19:16
Copy link
Contributor

@msivasubramaniaan msivasubramaniaan left a comment

Choose a reason for hiding this comment

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

LGTM

@msivasubramaniaan msivasubramaniaan merged commit 1dff159 into redhat-developer:main Jun 30, 2022
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.

4 participants