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/gopls: add .if and .not snippets for bool types #47545

Open
hhhapz opened this issue Aug 5, 2021 · 8 comments
Open

x/tools/gopls: add .if and .not snippets for bool types #47545

hhhapz opened this issue Aug 5, 2021 · 8 comments

Comments

@hhhapz
Copy link

@hhhapz hhhapz commented Aug 5, 2021

Is your feature request related to a problem? Please describe. The
introduction of postfix snippets has brought very convenient functionality to
gopls, especially when working outside of Goland. I wanted to contribute more
ideas for postfix completions inspired from what's already available from their
editors. In particular, I wanted to propose having a if! and not! prompts,
which do this (respectively):

(complex bool expression).if|
if (complex bool expression) {
	|
}

(expr).not|
!(expr)

Describe the solution you'd like
Implementing these in completion/postfix_snippets.go.

Describe alternatives you've considered
As a vim developper personally, I use
snippetsEmu. Other
potential alternatives are available in VSCode and Goland.

@gopherbot gopherbot added this to the Unreleased milestone Aug 5, 2021
@hhhapz hhhapz changed the title x/tools/gopls: add .if and .not snippets for bool types proposal: x/tools/gopls: add .if and .not snippets for bool types Aug 5, 2021
@findleyr findleyr removed this from the Unreleased milestone Aug 6, 2021
@findleyr findleyr added this to the gopls/unplanned milestone Aug 6, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 6, 2021

CC @muirdm.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 6, 2021

Thanks for the suggestion. Muir might have some thoughts on this.

@hhhapz
Copy link
Author

@hhhapz hhhapz commented Aug 6, 2021

Wanted to add, that we could totally over-engineer the not by handling stuff like ==, >= ^, etc specially but I personally think it's overkill. Most of the time (at least personally) that this is useful is just negating a boolean var or a complex boolean expression surrounded by parenthesis.

In my mind, for the edge cases:

if x == y.not

should expand to

if x == !y

This helps keep the completion snippets more light weight, and if the user so desires, they can get the functionality by surrounding the expression with parentheses, or alternatively manually changing it.

@muirdm
Copy link

@muirdm muirdm commented Aug 10, 2021

The if! snippet sounds reasonable, but you will need to do some work to make a == b.if! be handled as (a == b).if! (i.e. to apply to the entire statement instead of the selector expression). You also may run into issues with the keyword if as a selector.

I'm less convinced about the not! snippet. As described it would only apply to bool expressions, so for example it wouldn't work in cases like if a == "foo".not. If it did work in cases like that, then it would be hard to predict what it would do in more complex cases like if (a == b) == (c == d).not. Does Goland offer a "not" snippet?

@ianlancetaylor ianlancetaylor changed the title proposal: x/tools/gopls: add .if and .not snippets for bool types x/tools/gopls: add .if and .not snippets for bool types Aug 11, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 11, 2021

I don't think this needs to go through the proposal process, so taking it out. Please comment if you disagree.

@hhhapz
Copy link
Author

@hhhapz hhhapz commented Aug 13, 2021

You also may run into issues with the keyword if as a selector.

Sorry, I am not entirely sure I understand what you mean here.

Goland's implementation for these snippets is actually a bit complex. When there is an abiguous case, like "a == b.not!", it shows a context menu to let the user pick whether to only do the first part, or the entire thing:
image
image

From my understanding (correct me if I am wrong) is not possible with gopls atm, so I think it would be reasonable to ditch the .not snippet unless you have a suggestion on how we can circumvent this?

@muirdm
Copy link

@muirdm muirdm commented Aug 13, 2021

You also may run into issues with the keyword if as a selector.

Sorry, I am not entirely sure I understand what you mean here.

The Go parser has trouble with keywords showing up in unexpected/invalid places (i.e. the "if" in foo || bar.if). Gopls has a workaround that tries to make "foo.if" parse the "if" as a plain identifier rather than an if statement, but I'm not sure if it will work in the case of the new snippet.

From my understanding (correct me if I am wrong) is not possible with gopls

LSP snippets have choices which might allow for a similar experience. But simpler and better, probably, would be just to have gopls offer multiple completion candidates, one for each possibility.

Regardless, I would skip not and start with if first because it seems more useful and straightforward.

@hhhapz
Copy link
Author

@hhhapz hhhapz commented Aug 13, 2021

Sounds good. I will take a stab at it. Will report back if I come across any difficulty 😄

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

Successfully merging a pull request may close this issue.

None yet
6 participants