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

x/tools/internal/lsp: consider removing constant value from completion candidates #31923

Closed
muirdm opened this issue May 8, 2019 · 16 comments
Closed
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@muirdm
Copy link

muirdm commented May 8, 2019

Currently completion candidate labels for constants include the constant value. I personally find this distracting since the value of a constant is almost never material when choosing completions (i.e. you choose based on the constant name, not the value).

I polled the Slack #tools channel and everyone who responded agreed with this sentiment. Therefore, I propose we remove the constant's value from the label.

Also, the label is especially unhelpful for floating point constants:
Screen Shot 2019-05-08 at 1 24 31 PM

@gopherbot gopherbot added this to the Unreleased milestone May 8, 2019
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label May 8, 2019
@bhcleek
Copy link
Contributor

bhcleek commented May 9, 2019

It would be nice to keep the value in the details property, though, so it can be shown in the preview windows.

@muirdm
Copy link
Author

muirdm commented May 9, 2019

It would be nice to keep the value in the details property, though, so it can be shown in the preview windows.

Are you referring to the "detail" field of the completion candidate (the "float64", "int", "string" info in my screenshot), or do you mean the hover details, or something else?

@bhcleek
Copy link
Contributor

bhcleek commented May 9, 2019

The detail field of the completion candidate.

@muirdm
Copy link
Author

muirdm commented May 9, 2019

Do you have an example of when that would be useful? For me that would be almost equivalent to being in the label since the detail shows up right next to the label.

@bhcleek
Copy link
Contributor

bhcleek commented May 9, 2019

In vim-go the detail field is used to populate the info property, which is then used to populate the preview window.

@muirdm
Copy link
Author

muirdm commented May 9, 2019

Can you give a screenshot of the preview window? I'm not familiar enough with vim to know what that refers to.

Do you have an example of a constant where it is useful to see its value in the preview window?

@bhcleek
Copy link
Contributor

bhcleek commented May 9, 2019

Here's a preview window for autocompleting a constant:
image

I don't have a specific example of a constant where its useful to see its value, I just believe that generally it would be useful to see its value in the preview window. Very generally, I would like the gopls candidates' label field to hold the full name of the thing that will be completed. For function and methods, that should be function name and its parameter list (i.e. no return value). For variables and constants, that should just be their name. The details field should contain more. (e.g. a full function signature with return values for functions; the same + the receiver information for methods; names, values and types for constant;, names and types for variables; etc.)

FYI, here's a screenshot for completing a method:
image

@muirdm
Copy link
Author

muirdm commented May 9, 2019

Thanks, that is helpful. It doesn't look like the details are currently showing up at all for you, though. In my screenshot, the Go types that show up are the candidate "detail". The function/constant/method indicator is the candidate "kind".

Anyway, I agree that the label should reflect what will get inserted if you select that completion. I'm still not sure there is a use case for showing constant values in the detail, but if that is something people want we can always add an "expanded detail" config param.

The details field should contain more. (e.g. a full function signature with return values for functions

The detail for functions already contains the return values. See my below screenshot. Having the label followed by the details gives you the signature (minus the receiver).

Screen Shot 2019-05-09 at 9 03 07 AM

@muirdm
Copy link
Author

muirdm commented May 9, 2019

@stamblerre what do you think about constant values in completion candidates?

@bhcleek
Copy link
Contributor

bhcleek commented May 9, 2019

The details are literally all that's in the preview window (the preview window is the window at the top of each screenshot, not the autocompletion candidate window in my screenshots)

@bhcleek
Copy link
Contributor

bhcleek commented May 9, 2019

The detail for functions already contains the return values.
It only contains the return values from what I've seen. I think it should include more because 👇

Having the label followed by the details gives you the signature (minus the receiver).
It's quite difficult to know how to properly wrap the return values that's currently in details with parentheses when trying to concatenate the item and the details for functions or methods, because:

  1. return values should only be wrapped in parentheses when there's more than one return value
  2. determining whether there's more than one return value is difficult in the face of return values that are function types that take more than one parameter.

gopls can simply this a lot by making the details be more complete description instead of just what the item evaluates to.

But I feel like this is perhaps digressing into an issue that I've been intending to create to rethink the details field generally. Sorry for that; I can create a separate issue if you'd like to talk about the wider issues so we can keep this focused on how to manage the details field for const types.

@muirdm
Copy link
Author

muirdm commented May 9, 2019

the preview window is the window at the top of each screenshot

Haha, sorry. Didn't notice. What you are saying makes a lot more sense having the details and labels separated like that.

rethink the details field generally

Yeah I agree there are lots of cool/useful things we could do. We could also consider adding any extra detailed info into the "Documentation" field in addition to any actual docs, but that might not make sense.

I can create a separate issue if you'd like to talk about the wider issues so we can keep this focused on how to manage the details field for const types.

Yes, please. I'd like to start with removing constant values from the label if no one objects to that in particular.

@stamblerre
Copy link
Contributor

I had the constant values in the label because it's something we have internally in Google, and I hadn't heard any complaints. Do you think it would be worth keeping it in the detail? I am not sure how it would look there, since in VSCode, detail is on the right of the label.

I would support removing it if we can't imagine a single case where it might be useful. I can only think that maybe with string constants it could be helpful?

@muirdm
Copy link
Author

muirdm commented May 10, 2019

Do you think it would be worth keeping it in the detail?

I think would be pretty equivalent to being in the label since (in most editors) the detail shows up right next to the label, albeit somewhat deemphasized.

maybe with string constants it could be helpful?

Yeah, I could imagine a case where you are composing a bigger string using string constants and it might be useful, but I can't think of any real examples. Regardless, if 99/100 times the constant value is extra noise, I'd still be in favor of getting rid of it.

@stamblerre
Copy link
Contributor

Absolutely agreed. Let's remove it, and if anyone complains and offers a valid counter example, perhaps we can consider adding a special case for it.

@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label May 14, 2019
@gopherbot
Copy link

Change https://golang.org/cl/177357 mentions this issue: internal/lsp: remove constant value from label and add tests

@golang golang locked and limited conversation to collaborators May 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants