improvement:don't show types for match case for showInferredType #5284
Conversation
| @@ -488,7 +488,8 @@ final class SyntheticsDecorationProvider( | |||
| case param: m.Term.Param => | |||
| if (param.decltpe.isEmpty) List(param.name.pos.toSemanticdb) else Nil | |||
| case cs: m.Case => | |||
| explorePatterns(List(cs.pat)) ++ visit(cs.body) | |||
| // if the case is too long then it'll be too messy | |||
| visit(cs.body) // explorePatterns(List(cs.pat)) ++ visit(cs.body) | |||
There was a problem hiding this comment.
I don't agree about removing this, since it might be useful to know the type in case.
We might be over eager in case of inferred type for methods, we could for sure separate it under another flag (or rethink the settings here to be more useful).
Any other opininos?
There was a problem hiding this comment.
It could also be nice to provide more options like showInferredType = minimal| full | etc...
There was a problem hiding this comment.
Totally forgot about your PR, sorry about that! I think that's a smart idea with switching the setting. This might require some tinkering on Vs Code sie, but can be done later on without na issue.
I would however add a test case for this option in SyntheticDecorationsLspSuite, just repeat the 54-89 lines with the setting switched.
metals/src/main/scala/scala/meta/internal/decorations/SyntheticsDecorationProvider.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/decorations/SyntheticsDecorationProvider.scala
Show resolved
Hide resolved
…csDecorationProvider.scala Co-authored-by: Tomasz Godzik <[email protected]>
|
@tgodzik I just added a test in SyntheticDecorationsLspSuite |
|
Thanks! We seem to have a lot tests failing in the PR, but it's not clear to me why. I will try and rerun and if that doesn't help I will take a look next week. |
|
Ok cool, seems that the tests were flaky. I will try and get this merged after the next release. I am stoping merging anything but fixes until the release next week. |
previously, showInferredType will show types even for long cases like below:
if we don't show cases the effect is like below(this PR):
In my opinion, showing too much information like everything in match case isn't very helpful, but showing types elsewhere is quite nice. although we can toggle showInferredType, it's a bit tedious to do so and I end up not using this functionality.
feel free to suggest edits (maybe only enable showInferredType when case contains one field?)