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.

Sign up for GitHub

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

Jump to bottom

Parameter inlay hint separate from variable type inlay? #2876

Closed
DianaNites opened this issue Jan 19, 2020 · 7 comments
Closed

Parameter inlay hint separate from variable type inlay? #2876

DianaNites opened this issue Jan 19, 2020 · 7 comments
Labels
E-has-instructions Issue has some instructions and pointers to code to get started E-medium good first issue

Comments

@DianaNites
Copy link
Contributor

Would it be possible for these to be separate options? I'm not a big fan of the type inlays, I prefer hovering if I need them, but the recently added parameter name inlay is super nice
@matklad
Copy link
Member

matklad commented Jan 19, 2020

Yes, we should split this into two options

@SomeoneToIgnore SomeoneToIgnore mentioned this issue Feb 17, 2020
Closed
@Veetaha
Copy link
Contributor

Veetaha commented Feb 19, 2020

We should add appropriate option to config in package.json
https://github.com/rust-analyzer/rust-analyzer/blob/61fdcc5ded44d4a7300b19cf946acc30367dcbd2/editors/code/package.json#L166
Add getter for it in config.ts
https://github.com/rust-analyzer/rust-analyzer/blob/61fdcc5ded44d4a7300b19cf946acc30367dcbd2/editors/code/src/config.ts#L151-L152
And change this enabled flag to carry info about which type of inlay hints is enabled.
https://github.com/rust-analyzer/rust-analyzer/blob/61fdcc5ded44d4a7300b19cf946acc30367dcbd2/editors/code/src/inlay_hints.ts#L66

@lnicola lnicola added the E-has-instructions Issue has some instructions and pointers to code to get started label Feb 19, 2020
@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Feb 19, 2020 •
edited
Loading

We need to go deeper, to RA level otherwise RA will be computing all types of inlay hints anyway which might be suboptimal.

On a server level, the hints have a kind: https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_ide/src/inlay_hints.rs#L15

The setting needs to be passed there, and the logic altered so that the unnecessary kinds of hints are not computed.

@Veetaha
Copy link
Contributor

Veetaha commented Feb 19, 2020 •
edited
Loading

@SomeoneToIgnore does it make sense to separate inlayHints request to inlayTypeHints and inlayParameterHints? Though if both of them are enabled we would need to make two passes or are type and parameter hints independent anyway?

@SomeoneToIgnore
Copy link
Contributor

we would need to make two passes

Exactly, so I would rather have a single request and a few if's in the inlay_hints.rs instead.

@SomeoneToIgnore SomeoneToIgnore mentioned this issue Feb 24, 2020
Closed
@slyngbaek
Copy link
Contributor

I'll take a look into this one.

slyngbaek added a commit to slyngbaek/rust-analyzer that referenced this issue Mar 10, 2020
@slyngbaek
Parameter inlay hint separate from variable type inlay? rust-lang#2876
010b0bd 
Add setting to allow enabling either type inlay hints or parameter
inlay hints or both. Group the the max inlay hint length option
into the object.

- Add a new type for the inlayHint options.
- Add tests to ensure the inlays don't happen on the server side
slyngbaek added a commit to slyngbaek/rust-analyzer that referenced this issue Mar 10, 2020
@slyngbaek
Parameter inlay hint separate from variable type inlay? rust-lang#2876
e98aff1 
Add setting to allow enabling either type inlay hints or parameter
inlay hints or both. Group the the max inlay hint length option
into the object.

- Add a new type for the inlayHint options.
- Add tests to ensure the inlays don't happen on the server side
bors bot added a commit that referenced this issue Mar 12, 2020
@bors @slyngbaek
Merge #3543
d98a5fa 
3543: Parameter inlay hint separate from variable type inlay? #2876 r=matklad a=slyngbaek

Add setting to allow enabling either type inlay hints or parameter
inlay hints or both. Group the the max inlay hint length option
into the object.

- Add a new type for the inlayHint options.
- Add tests to ensure the inlays don't happen on the server side

Co-authored-by: Steffen Lyngbaek <steffenlyngbaek@gmail.com>
@Veetaha
Copy link
Contributor

Veetaha commented Mar 14, 2020 •
edited
Loading

@matklad #3543 should've closed this, thanks to slyngbaek!

JoshMcguigan pushed a commit to JoshMcguigan/rust-analyzer that referenced this issue Apr 1, 2020
@slyngbaek @JoshMcguigan
Parameter inlay hint separate from variable type inlay? rust-lang#2876
e07db6c 
Add setting to allow enabling either type inlay hints or parameter
inlay hints or both. Group the the max inlay hint length option
into the object.

- Add a new type for the inlayHint options.
- Add tests to ensure the inlays don't happen on the server side
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
E-has-instructions Issue has some instructions and pointers to code to get started E-medium good first issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@lnicola @slyngbaek @matklad @SomeoneToIgnore @DianaNites @Veetaha