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

Linting makes editing hard #963

Closed
deathaxe opened this issue Jan 17, 2021 · 15 comments · Fixed by #1008 or enxio/lsp4xml#129
Closed

Linting makes editing hard #963

deathaxe opened this issue Jan 17, 2021 · 15 comments · Fixed by #1008 or enxio/lsp4xml#129
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@deathaxe
Copy link

grafik

The screenshot shows a simple file which only misses the end tag </xs:schema>. This little issue results in the whole document being marked invalid, which doesn't help much. The file becomes harder to read while editing and doesn't tell much about the origin of the error.

I'd expect only the opening tag name and/or the end of the document being marked invalid. The content in between is very correct.

@angelozerr
Copy link
Contributor

This behavior comes from #897 (comment)

The main idea is to highlight the error from the start tag which is broken to the offset reported by Xerces:

I'd expect only the opening tag name and/or the end of the document being marked invalid

Ideally it should be nice to highlight the start/end tag but diagnostic support just one range.

@deathaxe
Copy link
Author

When I was working on the xsd file huge blocks of the document were highlighted this way while adding or rearranging tags. Even when just writing down a new document from top to down one ends up in huge blocks being marked this way.

I don't find it useful and very distracting.

Maybe the client can detect large regions and disable inline markup.

@angelozerr
Copy link
Contributor

When I was working on the xsd file huge blocks of the document were highlighted this way while adding or rearranging tags.

You should have code action which should help you to fix broken xml.

I don't find it useful and very distracting.

Ok thanks for your feedback, I will see how we could improve your usecase.

@deathaxe
Copy link
Author

Yes, code actions are available. Haven't used them much though as it always involves using the mouse to hover the annotation and selecting the desired ones. It seems to work in general though.

Most things can quickly be fixed via auto-completions which proposes correct closing tags. Not a problem. It's just the fact huge blocks of the document being marked most the time while working on a document. It's so to say a normal use case to have broken XML during authoring.

@predragnikolic
Copy link

Hello,

Ideally it should be nice to highlight the start/end tag but diagnostic support just one range.

Maybe diagnostic.relatedInformation: DiagnosticRelatedInformation[] may help for this use case? Here is an example.

Haven't used them much though as it always involves using the mouse to hover the annotation and selecting the desired ones. It seems to work in general though.

@deathaxe, you can assign a keyboard shortcut to trigger code actions.
Because I know you use Sublime text, here is it :)

{ "keys": ["ctrl+."], "command": "lsp_code_actions" },

@angelozerr
Copy link
Contributor

Maybe diagnostic.relatedInformation: DiagnosticRelatedInformation[] may help for this use case?

At first relatedInformation is not supported by all LSP client (ex: Eclipse IDE doesn't support it). We use relatedInformation when you have a XML bind with a XSD file which is not well formatted.

@angelozerr
Copy link
Contributor

We cover the range from start tag to offset reported by xerces to support correctly code action.

For instance with this XML:

<schema attr

xerces report the offset after attr. We can provide code actions after this attr content with /> or ></schema>.

In old version the range was just the start tag name (schema) and code actions inserted /> or ></schema> before the attr.

The main problem is that code action uses the error range to retrieve the offset where /> or ></schema> must be inserted. I know LSP 3.16 specify that code action can have a data.

When a diagnostic will be generated we could create a range with start tag name and set in the data the offset where xerces has reported the error.

@angelozerr
Copy link
Contributor

Ideally it should be nice to highlight the start/end tag but diagnostic support just one range.

Maybe diagnostic.relatedInformation: DiagnosticRelatedInformation[]

@predragnikolic thanks a lot for your suggestion, I will try to investigate your idea.

@deathaxe is Sublime LSP client can support relatedInformation?

@deathaxe
Copy link
Author

If not yet, it may learn it. That's what I read from @predragnikolic's suggestions. I am not yet very familiar with that approach, but the main question might be whether the result is something common which all language servers make use of, or whether the solution will be something specific. Not sure how to handle the latter case.

@predragnikolic
Copy link

Here is how other language servers handle that case.
The TypeScript server generates 2 diagnostics:

  • one at the open tag location
  • and the other at the end where the missing tag is expected.

examplee

The main problem is that code action uses the error range to retrieve the offset where /> or > must be inserted. I know LSP 3.16 specify that code action can have a data.

I am not sure If having 2 diagnostics, like in the image above, would provide enough information for the server to calculate where to put the closing tag.

@predragnikolic
Copy link

Does the Sublime LSP client support relatedInformation?

Yes, LSP for Sublime Text does support it.

Here is an example gif

Keep in mind that I hard coded the response to include the diagnostic.relatedInformation field, so this is actually not provided by the language server, but rather just for demonstrating purposes.

Sublime Text just shows related diagnostics information in the hover popup, when hovering over the diagnostics.

output

    diagnostic['relatedInformation'] = [{
            'location': {
                'uri': '/home/predrag/Documents/modulll/src/App.jsx',
                'range': {
                    'start': { 'line': 9, 'character': 15 },
                    'end' : { 'line': 9, 'character': 15 }
                },
            },
            'message': 'Missing closing tag here'
    }]

VS Code show relatedInformation in the diagnostics panel and the hover popup.
image

@datho7561 datho7561 self-assigned this Mar 3, 2021
datho7561 added a commit to datho7561/lemminx that referenced this issue Mar 8, 2021
Need to fix current tests and add more tests

Closes eclipse-lemminx#963

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561
Copy link
Contributor

Something that may prevent this from being implemented is that we depend on the end of the error range in order to generate the CodeAction to close the element. This is no issue for clients that support relatedInformation, since the correct end tag location is in the relatedInformation. We can't take this approach in clients that don't support relatedInformation.

These are some options that I can see to move forward with this:

  1. Restructure LemMinX's CodeActions so they only depend on the start of the error range. This might take a while. It also means we have to calculate the close tag location twice: once for the diagnostic and once for the CodeAction.
  2. Don't support CodeActions at all on clients that don't support related info
  3. Keep the old diagnostic range on clients that don't support. This sounds like it would cause more headaches than its worth.

Please let me know if you can think of other ways to go about this.

@angelozerr
Copy link
Contributor

We can't take this approach in clients that don't support relatedInformation.

I think we should follow this idea. The diagnostics will work for any LSP client but for LSP client which support relatedInformation, the higlighting error will be better.

It looks like Outline with symbols. If LSP client can support symbol hierarchy, the Outline renderer will be better.

@datho7561
Copy link
Contributor

@angelozerr so do you mean option 3?

@angelozerr
Copy link
Contributor

If old in "Keep the old diagnostic range on clients that don't support." means current behavior, I would say "yes"

datho7561 added a commit to datho7561/lemminx that referenced this issue Mar 23, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired.

Closes eclipse-lemminx#963
datho7561 added a commit to datho7561/lemminx that referenced this issue Mar 23, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired.

Closes eclipse-lemminx#963
datho7561 added a commit to datho7561/lemminx that referenced this issue Mar 23, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact.

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired errors.

Closes eclipse-lemminx#963
datho7561 added a commit to datho7561/lemminx that referenced this issue Mar 23, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact.

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired errors.

Closes eclipse-lemminx#963
datho7561 added a commit to datho7561/lemminx that referenced this issue Mar 23, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact.

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired errors.

Closes eclipse-lemminx#963
datho7561 added a commit to datho7561/lemminx that referenced this issue Mar 23, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact.

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired errors.

Closes eclipse-lemminx#963
datho7561 added a commit to datho7561/lemminx that referenced this issue Mar 23, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact.

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired errors.

Closes eclipse-lemminx#963
@datho7561 datho7561 added the enhancement New feature or request label Mar 24, 2021
@datho7561 datho7561 added this to the 0.17.0 milestone Apr 13, 2021
datho7561 added a commit to datho7561/lemminx that referenced this issue Apr 30, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact.

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired errors.

Closes eclipse-lemminx#963
datho7561 added a commit to datho7561/lemminx that referenced this issue Apr 30, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact.

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired errors.

Closes eclipse-lemminx#963
angelozerr pushed a commit that referenced this issue Apr 30, 2021
Revert MarkupEntityMismatch and ETagRequired error ranges so they only
cover the start tag element name.

Keep existing CodeAction behaviour intact.

If the client supports DiagnosticRelatedInformation, add the expected
location of the close tag as related info to MarkupEntityMismatch and
ETagRequired errors.

Closes #963
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants