Fixed FileCompletionItem to replace the entire node value for cleaner completion

[pauldpong]

• Changed how we got the prefix for autocompletion, no longer need to do substring of the entire document.
• toFileCompletionItem will now attempt to get the full range of the node value, and replace the entire value rather than what is returned by request.getReplaceRange

[mickaelistria]

Can you please add a test that reproduces the issue ans ensure the patch fixes it?

[angelozerr]

I wonder if this problem comes from LemMinx eclipse-lemminx/lemminx#723 ?

[pauldpong]

Can you please add a test that reproduces the issue ans ensure the patch fixes it?

Yep, added a test that make sure that the replace range for file completion is the entire text node. @mickaelistria

I wonder if this problem comes from LemMinx eclipse/lemminx#723 ?

I believe so. Should this fix go through as a temporary fix or should we just wait for that bug to be fixed?

[angelozerr]

I believe so. Should this fix go through as a temporary fix or should we just wait for that bug to be fixed?

IMHO, I think the bug should be fixed in LemMinx, I have add a comment in
eclipse-lemminx/lemminx#723 (comment)

[mickaelistria]

Should this fix go through as a temporary fix or should we just wait for that bug to be fixed?

We should never build workarounds if we know a better and affordable way to have a propoer fix.
If you're interested, you can try to submit a PR for the LemMinX bug. Or maybe @AObuchow is insterested?

[AObuchow]

@mickaelistria I'll try doing an upstream fix for this

[AObuchow]

Perhaps we should remove the workaround from this patch, keep the test but disable it with JUnit @ ignore, and then I can validate the upstream fix addresses the issue and reenable the test?

[mickaelistria]

Let's comment where the workaround is with a link to upstream issue, and a note that this can be removed when we move to newer lemminx. Moving to newer LemMinX requires also orchestration with a release of Wild Web Developer, so we can't just switch it too fast without planning.

[AObuchow]

Let's comment where the workaround is with a link to upstream issue, and a note that this can be removed when we move to newer lemminx. Moving to newer LemMinX requires also orchestration with a release of Wild Web Developer, so we can't just switch it too fast without planning.

Sounds good to me, @pauldpong would you mind updating the PR accordingly with a link to eclipse-lemminx/lemminx#723 (comment)? We can then merge this finally

[pauldpong]

Let's comment where the workaround is with a link to upstream issue, and a note that this can be removed when we move to newer lemminx. Moving to newer LemMinX requires also orchestration with a release of Wild Web Developer, so we can't just switch it too fast without planning.

Sounds good to me, @pauldpong would you mind updating the PR accordingly with a link to eclipse/lemminx#723 (comment)? We can then merge this finally

Done!

[AObuchow]

Looks great to me, thanks @pauldpong !!! :D