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

VSCode extension: Useless inlay hints #16

Closed
Tracked by #29
MDLC01 opened this issue Mar 11, 2024 · 9 comments · Fixed by #37
Closed
Tracked by #29

VSCode extension: Useless inlay hints #16

MDLC01 opened this issue Mar 11, 2024 · 9 comments · Fixed by #37

Comments

@MDLC01
Copy link

MDLC01 commented Mar 11, 2024

Describe the bug

Sometimes, inlay hints are displayed even when not necessary.

To Reproduce

  1. Create a new Typst file with the following contents.

    #let f(arg: none) = {}
    #f(arg: "hey")
    $ cal(M) $
  2. Notice how hints appears after "hey" and M.

Expected behavior

The hint after "hey" should not exist, because the this is a named parameter and the hint is therefore redundant.

The hint after M is useless ("body" does not convey any additional information), and it takes up horizontal space for nothing. I think inlay hints should not appear for arguments named body (and maybe some other highly generic names).

Additionally, I think there should be a way to disable inlay hints, as some people don't like this feature.

Package/Software version

tinymist extension version: v0.10.1

@Myriad-Dreamin
Copy link
Owner

Myriad-Dreamin commented Mar 11, 2024

I'm going to remove the inlay hints on named arguments and add a flag to disable inlay hints dynamically.

I think inlay hints should not appear for arguments named body (and maybe some other highly generic names).

but I'm not sure how to process the positional arguments. It is not suitable to determine whether a name is general enough by lsp, which highly depends on personal sense. That means if you don't like positional arguments, you should disable it totally.

@MDLC01
Copy link
Author

MDLC01 commented Mar 11, 2024

I was mainly thinking about "body", since this is a very common argument name. But I understand why you wouldn't want to add a list of exceptions.

Maybe there should be a way to disable inlay hint only in math mode, though.

@ntjess
Copy link

ntjess commented Mar 12, 2024

Just for reference, here is vscode Python's approach to "partial" inlays:
image

But this is not helpful for Typst since all arguments are strictly positional or strictly keyword

Pycharm provides inlays for literal positional arguments, assuming variable names are self-documenting. I'm a fan of this approach, but still has the problem for "body" arguments as mentioned in this issue

Perhaps if a function takes one argument, it is assumed to be obvious, but two or more positional, literal values will get the hints

I don't think hard-coding assumptions based on the name is the way to go. There are several conventions (it, body, content, ...) and it may be beneficial to inlay body when it comes after a different argument. I use the following for slides quite often:

let my-slide(title, body) = {...}

And it's nice to see inlay confirmation that "title" comes before "body" in this simple example without referring to the function docs

Just a few thoughts.

@MDLC01
Copy link
Author

MDLC01 commented Mar 12, 2024

Perhaps if a function takes one argument, it is assumed to be obvious, but two or more positional, literal values will get the hints

I like that!

@Myriad-Dreamin
Copy link
Owner

Myriad-Dreamin commented Mar 12, 2024

After thinking of it a bit, I can provides two special patterns of code to enable/disable individually:

  • onSinglePositionalArgument: when the function has only a single positional parameter.
  • onContentBlockArgument: when you use content[] grammar to pass the content arguments.

The inlay hints will not show if any pattern hits.

This is how to configure it. First, we have the simple switch to enable/disable/partially enable inlay hints.

{
  "tinymist.inlayHints": "on" | "off" | "smart" // default: smart
}

I prefer to use smart rather than partial, because

  • I encourage you to use it, that sometimes help find bugs in your code.
  • partial are not descriptive, so I can change a name by my preference.

And then you can configure (like how you controls margins/paddings x, y, left, right, top, bottom of typst nodes) it by an object:

{
  "tinymist.inlayHints": {
    "onSinglePositionalArgument": "off",
    "onContentBlockArgument": "on",
    "rest": "on" // set default values to on or off
  }
}

@MDLC01
Copy link
Author

MDLC01 commented Mar 12, 2024

To be clear,

{
  "tinymist.inlayHints": "smart"
}

is equivalent to

{
  "tinymist.inlayHints": {
    "onSinglePositionalArgument": "off",
    "onContentBlockArgument": "on",
    "rest": "on"
  }
}

?

If so, I like the customizability, and the fact that you get a sensible behavior by default with "smart".

@Myriad-Dreamin
Copy link
Owner

Myriad-Dreamin commented Mar 13, 2024

To be clear,

{
  "tinymist.inlayHints": "smart"
}

is equivalent to

{
  "tinymist.inlayHints": {
    "onSinglePositionalArgument": "off",
    "onContentBlockArgument": "on",
    "rest": "on"
  }
}

?

If so, I like the customizability, and the fact that you get a sensible behavior by default with "smart".

Yes. The string values of the inlayHints configuration are just equivalent to some object representations.

[
// the following two are equivalent:
{ "tinymist.inlayHints": "on" }
{ "tinymist.inlayHints": { rest: "on" } }
// the following two are equivalent:
{ "tinymist.inlayHints": "off" }
{ "tinymist.inlayHints": { rest: "off" } }
// the following two are equivalent:
{ "tinymist.inlayHints": "smart" }
{
  "tinymist.inlayHints": {
    "onSinglePositionalArgument": "off",
    // "onContentBlockArgument": "on",
    "rest": "on"
  }
}
]

@Myriad-Dreamin
Copy link
Owner

Myriad-Dreamin commented Mar 13, 2024

I have found another design question today, that the on enum may be confusing, because people probably don't get that when you on the "inlayHints", then all positions of "inlayHints" are shown.

The alternative enumerate values to replace on | off | smart are all | never | smart or always | never | smart.

@Myriad-Dreamin Myriad-Dreamin mentioned this issue Mar 13, 2024
6 tasks
@MDLC01
Copy link
Author

MDLC01 commented Mar 13, 2024

I think "always" | "never" | "smart" is the clearer one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants