Skip to content

feat: Add vscode native onEnterRules #10384

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

Merged
merged 1 commit into from
Sep 29, 2021
Merged

Conversation

HKalbasi
Copy link
Member

This PR copy onEnterRules configurations from vscode-rust based on discussion in #6254

I understand that the ideal way is to use parser data for this, and probably all other things that language-config do. But since it can't be enabled (or at least it isn't enabled) by default in vscode, I think this is needed (until onEnter becomes official LSP and get vscode support).

People can still configure shortcut and use the parser based onEnter, so it wouldn't harm anyone.

@lnicola
Copy link
Member

lnicola commented Sep 28, 2021

Do these still not work when declared in the JSON?

@HKalbasi
Copy link
Member Author

I didn't try. Does it matter?

@lnicola
Copy link
Member

lnicola commented Sep 29, 2021

Not too much, but doing it the declarative way and having less client code seems slightly nicer. But I wouldn't block this PR on it.

Oh well...

r? @matklad

@matklad
Copy link
Member

matklad commented Sep 29, 2021

Oh, so we actually can do this programmatically? Then let's make sure that it is possible to enable only one of custom onEnter or native on enter, and use the native one by default.

That is, what we want to avoid is them both being enabled and fighting each other, which I beleive can happen if we just unconditionally enable this?

@lnicola
Copy link
Member

lnicola commented Sep 29, 2021

Can we actually check that ours is enabled?

@matklad
Copy link
Member

matklad commented Sep 29, 2021

Even if we don't, we can just add a config to disable registration of native handles.

@HKalbasi
Copy link
Member Author

They will fight each other? I checked and it was not a problem (And I expected it because shortcut eats enter key completely). I also checked the multi-cursor, which did not work properly, but I do not think it is due interference. I just checked simple cases, if you have a special case tell me to check.

That said, make it disable-able has no cost, if there is benefit in it I can do it.

By the way, I noticed that RA adds // comments as well but vscode-rust doesn't. Should I add it for consistency? TS doesn't add // so I'm not sure what is correct.

@matklad
Copy link
Member

matklad commented Sep 29, 2021

If ther's no conflict, then r=me!

Should I add it for consistency?

Probably not -- the logic for // is non-trivial -- the comment is added only if you are in the middle of the comment, or if threes' a trailing whitespace. This tries to make single-line comments not annoying.

@HKalbasi
Copy link
Member Author

If ther's no conflict, then r=me!

I think there isn't. But it is good to someone else double check it.

Probably not -- the logic for // is non-trivial -- the comment is added only if you are in the middle of the comment, or if threes' a trailing whitespace. This tries to make single-line comments not annoying.

👍


By the way, lsif PR is also ready for review.

@lnicola
Copy link
Member

lnicola commented Sep 29, 2021

bors r=matklad,lnicola

@lnicola lnicola changed the title Add vscode native onEnterRules feat: Add vscode native onEnterRules Sep 29, 2021
@bors
Copy link
Contributor

bors bot commented Sep 29, 2021

@bors bors bot merged commit 9e3e98d into rust-lang:master Sep 29, 2021
@Zenithsiz
Copy link

I believe this has recently made doc comments (///) auto continue for me when pressing enter (in 0.2.768), but I cannot find any way to turn it off, is there a setting (in either ra or vscode) I can set to disable the continuing behavior?

I attempted to check if the rust-analyzer.onEnter keybinding was set, but it had no keybinding, so I'm not sure what's causing this.

@HKalbasi
Copy link
Member Author

HKalbasi commented Oct 6, 2021

It isn't disable-able on RA (an option can simply added) and surprisingly nor on code.

I wonder why you want to disable it?

@Zenithsiz
Copy link

It's mostly about being accustomed to writing comments manually and how I typically write doc comments before definitions.

When creating a new item (be it a struct, function, trait, etc...), I typically first put a single line doc comment to explain what it is, and then I write it's definition. With the auto continue, after pressing enter I get another doc comment and have to first delete it before typing the definition.

I could probably write the definition first and then write the doc comment, but I think it helps me to follow the definition off the doc comment and not backwards.

@RalfJung
Copy link
Member

RalfJung commented Aug 3, 2022

I wonder why you want to disable it?

Many of my doc comments are no longer than 2 lines. This means the heuristic for adding /// is wrong around 50% of the time, which is pretty bad for a heuristic. Also most of the newlines in my doc comments are added by automatic reflowing (via the "rewrap" extension), which makes the RA comment insertion correct even less often.

Conversely, I often write long regular // comments, and RA does not usually add any automatic comment markers there, which is quite annoying. (I know I can coerce RA into adding them by pretending that I am in the middle of a comment rather than at the end of it, via adding a useless // line at the end, but that is not very ergonomic or discoverable.)

I wonder why you think that you can automatically infer whether the programmer wanted to continue the comment vs continue writing code after the comment? I don't think there is a reliable way to detect that, and any attempt at being 'smart' here will just frustrate programmers. The way // is treated IMO shows that -- there is no heuristic that is not annoying. Please leave the choice up to the programmer, by giving them two different 'enter' behaviors -- one that never adds any comments, and one that always does. (E.g., one could be enter and the other shift-enter, or vice versa.) This is common in other editors (e.g., KDE Kate).

Also see #12928.

@HKalbasi
Copy link
Member Author

HKalbasi commented Aug 3, 2022

Many of my doc comments are no longer than 2 lines. This means the heuristic for adding /// is wrong around 50% of the time, which is pretty bad for a heuristic. Also most of the newlines in my doc comments are added by automatic reflowing (via the "rewrap" extension), which makes the RA comment insertion correct even less often.

Enter is not the only key that can be used to finish the doc comments. Specially in my flow, signature then doc then body, enter is wrong and I should use arrow keys even if r-a doesn't add /// on enters. So 50% false positive is not for every one and I think your reasoning is flawed here.

That said, I really understand that some people may not like this behavior, and I have no objections against making it configurable. Ideally, it should be configurable at vscode level, because if you don't like this behavior in r-a, you probably don't like it in other languages (js, java, c#, ...) and in other language servers (rls) as well, and this PR changes is vscode specific, so I think it worth raising an issue there (they might now a way to disable it). Meanwhile it should be possible to add a configuration for it in r-a, I even tried to do it after @Zenithsiz comment, but it needed more effort than what I was able/interested to put on this.

@RalfJung
Copy link
Member

RalfJung commented Aug 3, 2022

Specially in my flow, signature then doc then body

So you are writing code not in the order in which it appears in the file?

Sure, you do you, but please don't assume everyone works like that. :)

Ideally, it should be configurable at vscode level,

If I understand correctly, what you are doing here is (ab)using the rules for adding indentation in new lines to also add comments. I am not objecting at all to adding indentation, that is almost always correct. The issue stems from using the auto-indent mechanism for something else, namely auto-commenting. A vscode setting would probably also remove the auto-indentation and that would not be the right behavior. (Or maybe I am misunderstanding, the implementation details of vscode are completely foreign to me.)

I am not using vscode for any other languages so I don't know how this is handled there.

@HKalbasi
Copy link
Member Author

HKalbasi commented Aug 3, 2022

but please don't assume everyone works like that. :)

Fair :) but down arrow key is still an option.

If I understand correctly, what you are doing here is (ab)using the rules for adding indentation in new lines to also add comments.

No, onEnterRules is specifically for this situation, and general indenting rules are under indentationRules. For example this is the configuration file for the js which is written by Microsoft itself, use onEnterRules for exact same propose. And a configuration can just disable rules that contains text appends, if some extensions depend on it for indentation.

@RalfJung
Copy link
Member

RalfJung commented Aug 3, 2022

Fair :) but down arrow key is still an option.

Not sure what you mean, how can I use the down arrow key to add a newline without adding //!? I am here:

///! My great comment.<|>

and want to go on typing \nfn my_great_function.

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 this pull request may close these issues.

5 participants