search

eclipse / lsp4e

Language Server Protocol support in Eclipse IDE
Eclipse Public License 2.0
61 stars 54 forks source link

feat: make diagnostic marker messages configurable #1066

Closed sebthom closed 2 weeks ago

sebthom commented 3 weeks ago

This PR extends the IMarkerAttributeComputer with a method computeMarkerMessage(Diagnostic) allowing language server providers to customize the message to be used for markers.

travkin79 commented 3 weeks ago

Hi @sebthom, it's great to see this PR, since I was working more or less on the same topic.

For CDT LSP, I'd like to add problem issue codes and problem description URLs (that the language server receives from clang-tidy, e.g. something like readability-magic-numbers and https://clang.llvm.org/extra/clang-tidy/checks/readability/magic-numbers.html) to the problems view and / or to the markers view. My idea was to add new columns Code and URL to these views. But I came across an extension issue and was working an a PR to fix it.

As long as that fix is not available, using the code as message suffix is a good idea. But it would be even better if the diagnostics code and maybe other details could be presented in separate columns in the problems / marker views. Clients could add these or even more marker fields to these views, depending on the marker type and language (or LS capabilities) and the user could show or hide these columns on demand.

What do you think?

sebthom commented 3 weeks ago

I think it would be a nice addition but not as part of this PR. If it doesn't exist already please open a dedicated issue.

rubenporras commented 3 weeks ago

Hi @travkin79 ,

great news. I think that to add new columns Code and URL to these views is the right solution to the problem (and since we do not have that, we built our own views for our product (as I commented). Adding the information as a textual string looks like a poor man solution instead.

@sebthom: Since we have better solution already prepared for as 2024-12, it would be really be better if this feature would be opt-in. This way we can remove it later when we can add the attributes to the view and most users will not see a change in the way the marker messages are presented, only an improvement in the view.

travkin79 commented 3 weeks ago

Hi @sebthom,

I think it would be a nice addition but not as part of this PR.

Of course, not as part of this PR. 😄

If it doesn't exist already please open a dedicated issue.

It seems that issue #185 has a very similar goal. But it does not explicitly mention adding new columns "Code" and "Details URL" (or similar) to the problems view or to the marker view. Thus, I created a new issue #1070.

sebthom commented 3 weeks ago

@rubenporras I honestly don't see the point in making this an opt-in. realistically this will mean it will not get implemented for any LS in the near future. I still believe it is very valuable to see the actual diagnostic code directly in the hover message and not a distant list with unrelated messages. Esp. if the error code can be used to e.g. suppress warnings like in shellcheck using # shellcheck disable=SC2119,SC2120 or in Dart using // ignore: non_constant_identifier_names. Then you can simply F2 on the marker message and copy/paste the code into the comment.

travkin79 commented 3 weeks ago

@rubenporras, could it also be an opt-out?

I think, in most cases the diagnostics code will not be too long.

sebthom commented 3 weeks ago

Currently it is an opt-out. But as I said we could also change the implementation to omit codes if they are too long (whatever the definition of too long would be).

@rubenporras I actually am very curious how long the codes are in that language server you are working on (and why they are so long).

travkin79 commented 3 weeks ago

@sebthom, Could you also fix the hover dialog's size (maybe in another PR)? In the following example it's to small.

image

If extend it to the actual size it becomes the following.

image

rubenporras commented 3 weeks ago

My reasoning for an opt-in is that if you have the attribute in the markers view, you can also copy from there, so the information is redundant, and it makes the hover bigger, which might be a problem. As a side note, as far as I can tell, the JDT does not add the code to a message, and I think VS Code also does not do it. There might be reasons for it :)

In any case, I can customize the behaviour, so if I am the only one that does not like it, you can merge it. Maybe @mickaelistria has an opinion on the usability?

Regarding your question: Our codes are long because we bundle a lot of plug-ins for many DSLs (company made), to assure uniqueness we have the <"fully qualified name of the plug-in">. .

sebthom commented 3 weeks ago

Ok, there is no need to rush this PR and incorporate something that ends up being dead code in a few months. We can also wait if someone implements the problems view extra column and see if it sufficient.

rubenporras commented 3 weeks ago

@sebthom, Could you also fix the hover dialog's size (maybe in another PR)? In the following example it's to small.

If he knows how to fix it or has time to look into it, by all means. Otherwise I am already investigating this since a couple of days. If @sebthom are interested into working on it, I can share what I have found so far.

@travkin79 , also another feature that seems to be broken for me (and I believe has the same root cause), is that if you apply the quicfix by clicking in the hyperlink, the hover does not disappear. Could you confirm this for you environment as well?

sebthom commented 3 weeks ago

@rubenporras I have no experience with fixing hover heights so far and I am not working on that. so if you find a solution please contribute :)

travkin79 commented 3 weeks ago

Hi @rubenporras,

if you apply the quickfix by clicking in the hyperlink, the hover does not disappear. Could you confirm this for you environment as well?

Yes, it's the same issue in my environment. In addition, I cannot select the text from the diagnostics message in the hover dialog in the area highlighted in the following screenshot.

image

travkin79 commented 3 weeks ago

Hi @sebthom, Sorry for being a little off topic, but I don't know where to ask that: I have a missing dependency in LSP4E after pulling from main branch (see screenshot). I think, you're most familiar with null checks. :-) Is there an update site that I can use to install the missing bundle to my environment?

image

sebthom commented 3 weeks ago

it comes from maven central. the maven location is defined in the target platform. maybe you need to update/refresh/reload the target

travkin79 commented 3 weeks ago

I didn't use the target file, since I am testing a few additional plug-ins in the same workspace. Is there a simple way to add that library to Eclipse, maybe similar to the libs from orbit update sites?

sebthom commented 3 weeks ago

I didn't use the target file, since I am testing a few additional plug-ins in the same workspace. Is there a simple way to add that library to Eclipse, maybe similar to the libs from orbit update sites?

I have no idea. I only work with target files.

travkin79 commented 3 weeks ago

I have no idea. I only work with target files.

Well, then it might be a feature request for your project. 😄 I found this on contributing a library to orbit simultaneous releases: https://github.com/eclipse-orbit/orbit/blob/main/Add-bundle-in-5-minutes.md I'd appreciate it if you would consider contributing your library to the orbit update sites.

rubenporras commented 3 weeks ago

I have no idea. I only work with target files.

Well, then it might be a feature request for your project. 😄 I found this on contributing a library to orbit simultaneous releases: https://github.com/eclipse-orbit/orbit/blob/main/Add-bundle-in-5-minutes.md I'd appreciate it if you would consider contributing your library to the orbit update sites.

Probably you need install the maven integration plugin (M2E) for the target file to work properly.

rubenporras commented 3 weeks ago

Ok, there is no need to rush this PR and incorporate something that ends up being dead code in a few months. We can also wait if someone implements the problems view extra column and see if it sufficient.

I will do most probably a release of LSP4E before that happens, given that it will need to wait for 2024-12, and since the PR that adds the code to the message is merged, I need some kind of configuration.

I would say I merge this PR tomorrow if no-one disagrees, as it is an improvement over the existing code in master in any case.

travkin79 commented 3 weeks ago

Probably you need install the maven integration plugin (M2E) for the target file to work properly.

I have M2E installed. As soon as I activate the target platform, the issue disappears, but my other bundles do not compile anymore. That's why I need the library somewhere in Eclipse as an OSGi bundle. But since it is not required at runtime, I can work this way anyway.

Well, then it might be a feature request for your project. 😄 I found this on contributing a library to orbit simultaneous releases: https://github.com/eclipse-orbit/orbit/blob/main/Add-bundle-in-5-minutes.md I'd appreciate it if you would consider contributing your library to the orbit update sites.

@sebthom, I found a simpler solution. No need for putting your lib to orbit update sites (at least not for me). I just added the Maven dependency to a customized target definition in the preferences (without an explicit target definition file). That works for me.

sebthom commented 3 weeks ago

@mickaelistria based on the recent discussion, what do you think about this PR? should we merge it as is? make it opt-in or wait for another solution?

mickaelistria commented 3 weeks ago

If you think it's good to merge, and @rubenporras (and is "counter" use-case) agrees this PR is good as he did in https://github.com/eclipse/lsp4e/pull/1066#issuecomment-2302328649 , I don't see any reason why not merging it.