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

Completion should generate only start section when empty end section is found #807

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

angelozerr
Copy link
Contributor

@angelozerr angelozerr commented Mar 1, 2023

Generate only start section when empty end section is foundCompletion should generate only start section when empty end section is found

Fixes #805

Signed-off-by: azerr azerr@redhat.com

@angelozerr
Copy link
Contributor Author

@JessicaJHee here a new PR which do the samething than PR #806 but without updating end tag section (since {/} is a valid end section). Here a demo:

SnippetCompletionOnStartTag

@angelozerr angelozerr force-pushed the only-snippert-start branch 2 times, most recently from 61972cb to 2b9b3a1 Compare March 2, 2023 09:12
@angelozerr angelozerr marked this pull request as ready for review March 2, 2023 09:12
@angelozerr
Copy link
Contributor Author

The code of this PT should be clean, you can review it.

@datho7561
Copy link
Contributor

If you have an unnamed close tag right after what you're writing, then it will never generate a close tag for tag completion, even if there aren't enough unnamed close tags to match the content you are generating:

DoesntGenerateCloseTag

@angelozerr angelozerr force-pushed the only-snippert-start branch 2 times, most recently from ab20be8 to aa225e6 Compare March 2, 2023 19:37
@angelozerr
Copy link
Contributor Author

If you have an unnamed close tag right after what you're writing, then it will never generate a close tag for tag completion, even if there aren't enough unnamed close tags to match the content you are generating:

I have improved template parser to take care of this usecase. Please retry it.

@angelozerr
Copy link
Contributor Author

@JessicaJHee could you check if it is working with surround please.

@datho7561
Copy link
Contributor

Given the following template:



|{#each items}

{#each items}
  {it.name}
{/each}

{#each items}

{#each items}
  {it.name}
{/each}



{/}|

If I select between the two |, then I get a lot of code action failure notifications

Copy link
Contributor

@JessicaJHee JessicaJHee left a comment

Choose a reason for hiding this comment

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

LGTM, works well with surround with command! Thank you for writing lots of tests :)

@angelozerr angelozerr force-pushed the only-snippert-start branch 2 times, most recently from 8b63171 to cc64bdb Compare March 3, 2023 12:31
@angelozerr
Copy link
Contributor Author

According this comment it is not a good idea to change the behavior of the parser.

I revert my change and to fix the original issue

{#each items}
	{#|}
{/}

which didn't generate the full content of the snippet (only the first part of the section), I check if parent section of {# is closed. In this case, #each is not closed because the {/} is linked to {#. As parent section of {# (which is the #each) is not closed, the full content of the snippet must be generated.

@datho7561 please retry it and I suggest that you get redhat-developer/vscode-quarkus#585 to play with surround too.

@angelozerr
Copy link
Contributor Author

Here a demo:

SurroundSection

@datho7561
Copy link
Contributor

Seems pretty good so far, I'm going to try it out with redhat-developer/vscode-quarkus#585 and see how that goes...

Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

This works very well. I found a few spots where the code could be cleaned up a bit, but nothing major. Feel free to merge after you've addressed the comments.

Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks, Angelo!

@datho7561 datho7561 merged commit 8a0c934 into redhat-developer:master Mar 3, 2023
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.

Completion should generate only start section when empty end section is found
3 participants