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

Add automatic value completions on named arguments #827

Merged
merged 3 commits into from Sep 4, 2019

Conversation

@tgodzik
Copy link
Collaborator

commented Jul 12, 2019

I talked around in the office and someone said it would be useful to have something that autofills named parameters. After tinkering a bit with possibilities I come up with:

current-auto

What does anyone think? I was pretty easy to implement, but not sure about the ideal functionality. I am not sure about default arguments.

All suggestions are welcome.

@tgodzik tgodzik requested review from gabro and marek1840 Jul 12, 2019
@tgodzik tgodzik force-pushed the tgodzik:exuastive-named branch from e03f59b to 09be475 Jul 12, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

Also thinking of default values, some ideas:

  1. Do a default value ("" for string, false for Boolean etc.)
  2. Add ??? and a concrete value if the name is the same or is the only thing in scope
@marek1840

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

  1. Do a default value ("" for string, false for Boolean etc.)
  2. Add ??? and a concrete value if the name is the same or is the only thing in scope

I think that picking the variable "by name" from current scope is an awesome step in making the user experience much better.

As to the defaults, I think we would benefit from using the ??? for every parameter without matching variable in scope (same name and assignable type). The most important stuff: developers would be forced to provide a value, which would prevent various bugs (forgetting to change false to true, changing the 1 to i, that sort of stuff ;) ).

@tgodzik tgodzik force-pushed the tgodzik:exuastive-named branch 2 times, most recently from ddd715d to e803dde Jul 17, 2019
@tgodzik tgodzik changed the title [WIP] Add exhaustive matches on named arguments Add automatic value completions on named arguments Jul 23, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 14, 2019

Added snippets to navigate over autofilled arguments and fixed detection of same members:

snippets

@olafurpg

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Thank you @tgodzik for taking the initiative to implement this cool idea!

I am concerned about false positives. While this feature is useful in the scenarios where the guessed values are correct, the benefits of this feature may be outweighed by the frequent situation where the guessed values are incorrect and the user needs to manually edit the guessed values.

I am also concerned that users will blindly use guessed values like isAdult = false resulting in buggy programs.

At the price of a few additional keystrokes, the current behavior where users incrementally add one argument at a time offers a more flexible workflow where users don't have to spend mental energy wondering whether the "autofill" completion will work correctly at each call-site.

It's not clear to me what the expected behavior is in some scenarios:

  • should arguments be completed for parameters with default values? I would probably say we don't autocomplete parameters that already have default values.
  • should false or ??? be a default value for Boolean parameters? I would lean towards ???
  • should None or ??? be a default value for Option[T] parameters? I would towards ???.
  • should we support custom empty values for other Option-like data structures? I would lean towards not supporting custom empty value.

I'm curious to know what other people expect the behavior to be like.

Do you think implementing the "Use named argument for current and subsequent arguments" inspection in IntelliJ could reduce the need for this feature?

Screenshot 2019-08-25 at 12 55 13

|}
|""".stripMargin,
"""|argument = : Int
|argument = number : Int

This comment has been minimized.

Copy link
@olafurpg

olafurpg Aug 25, 2019

Member

I am concerned we're trying to be too smart in this particular case. I think it's fine to complete only one thing at a time, for example there may be variables in scope that start with arg*.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Aug 28, 2019

Author Collaborator

We first check for values with the name, then for named arguments, named arguments with values and the autocomplete is last.

I think that's the best option and is compatible with the current behaviour. Wouldn't cause users to need to scroll more then now to use the current capabilities.

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 28, 2019

I am concerned about false positives. While this feature is useful in the scenarios where the guessed values are correct, the benefits of this feature may be outweighed by the frequent situation where the guessed values are incorrect and the user needs to manually edit the guessed values.

I am also concerned that users will blindly use guessed values like isAdult = false resulting in buggy programs.

At the price of a few additional keystrokes, the current behavior where users incrementally add one argument at a time offers a more flexible workflow where users don't have to spend mental energy wondering whether the "autofill" completion will work correctly at each call-site.

I think we could drop the default literals and replace them with ??? this would make it clear that Metals don't want to guess the values. In all other scenarios we don't really guess, but just use the only available scope information, so the user would still can choose if there is more than one applicable value.

  • should arguments be completed for parameters with default values? I would probably say we don't autocomplete parameters that already have default values.

I think we should most likely use the default value, but not sure if we have access to it. So we could skip all those that have default values if there is no applicable value in scope.

  • should false or ??? be a default value for Boolean parameters? I would lean towards ???
  • should None or ??? be a default value for Option[T] parameters? I would towards ???.
  • should we support custom empty values for other Option-like data structures? I would lean towards not supporting custom empty value.

We might change that all to ???, since all literal values would be guessing. I agree

Do you think implementing the "Use named argument for current and subsequent arguments" inspection in IntelliJ could reduce the need for this feature?

I don't think we need that since the option to autofil will be last, so people should know what they are doing if scrolling down the list.

tgodzik added 3 commits Jul 12, 2019
…ompletion until =

Now, we add completions based on type and suggest possible values and add a possibility to fill all arguments with default values.
@tgodzik tgodzik force-pushed the tgodzik:exuastive-named branch from 59aa204 to ab71afe Aug 28, 2019
@tgodzik tgodzik requested review from olafurpg and marek1840 Aug 28, 2019
Copy link
Collaborator

left a comment

With the ??? as a fallback value I think, this can be considered completed.

@tgodzik tgodzik merged commit f83e896 into scalameta:master Sep 4, 2019
1 of 2 checks passed
1 of 2 checks passed
scalameta.metals Build #20190828.4 failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tgodzik tgodzik deleted the tgodzik:exuastive-named branch Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.