Fixed golang language server hover bug#9874
Conversation
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
|
ci-test |
|
Everything works as expected - hovers do appear and display info they should display - type, docs etc |
|
ci-test build report: |
|
Could you please state what the problem was and how this PR fixes it? I'd like to evaluate the potential impact on jdt.ls work. |
tsmaeder
left a comment
tsmaeder
left a comment
There was a problem hiding this comment.
I think this PR uses the wrong approach. We should configure hovers, etc. based on contributions. We have the possibility to contribute through configuration and through injection. Let's get rid of the default mappings.
| /** @author Dmytro Kulieshov */ | ||
| @Singleton | ||
| public class DefaultExtensionToMimeTypeMappings { | ||
| private final Map<String, Set<String>> mappings = |
There was a problem hiding this comment.
We can make this static, it's immutable for the lifetime of the class. Lazy initialization/dependency injection gives no advantage here.
There was a problem hiding this comment.
We can make this static, it's immutable for the lifetime of the class. Lazy initialization/dependency injection gives no advantage here.
Besides the fact that it preserves architectural homogeneity of the system and simplifies unit tests composition.
| /** The known extensions registry. */ | ||
| Map<String, List<String>> mappings = new HashMap<>(); | ||
|
|
||
| public ExtensionFileTypeIdentifier() { |
There was a problem hiding this comment.
It would have been a much simpler fix to simply register the mime types for hover here instead of refactoring stuff.
There was a problem hiding this comment.
As I remember we have already discussed this in another pull request, in general my attitude stays the same - my preference is to do it the most appropriate and simple way, not just simple way. If you experience difficulties in understanding I would recommend you to become more familiar with such concepts as decomposition, code refactoring and single responsibility principle. In our case there are several quite different components and their corresponding responsibilities: file type identification, default mime types registration, hover configuration initialization. Please just ping me if you need any other assistance.
@tsmaeder I find it strange that you've missed that during your review, but actually this pull request does not take away ability to configure hovers, it simply adds the default set of configurations for hovers, which may be (and for most supported languages is) overriden by contributed configurations (e.g. through injection). So when you say "we should configure hovers" and imply that this pull request disables that feature I find myself confused and puzzled. |
|
@tsmaeder I think this PR doesn't have any influence to the jdt.ls. Proposal provided by @dkuleshov just work and I don't see any technical mistake on it. We discus this solution internal before start implementation. If you have idea how to implement it in better way you can provide own PR |
|
We did not find regression in the functionality that is covered by selenium tests. Also we checked LS features according to manual testplan: https://github.com/eclipse/che/wiki/Test-plan-for-checking-Language-servers. The main features worked properly. |
|
What I was commenting on is that the default mime types were introduced as a workaround for the fact that we could not contribute mime types for language servers defined in the workspace config only. What I propose is that it would be more useful to remove the original limitation instead of fixing the workaround. |
|
Thinking about it overnight, I am wondering why we have a special entity for the default mime types. Why are we not simply contributing default LanguageDescription instances? Then all the hover-, etc. registration would be in one place. Methinks that would make the code simpler. |
That's a reasonable suggestion, my only concern is how we will distinguish the default Ideal solution I believe is to move language description to some configuration (like workspace configuration) so we could configure languages on demand w/o recompilation, however I have not clear vision on how we can do that now. WDYT @vparfonov |
|
For now i prefer current proposal in future will se maybe we will found better solution. Anyone can propose new PR. So goto merge this one |
benoitf
added
the
kind/bug
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
6 participants
Related issue #9781
Added default hover and occurrences provider initialization, moved default extension to mime type mappings to a separate class.