Skip to content

Conversation

SomeoneToIgnore
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore commented Aug 29, 2023

If I understand right, () can surround pretty much the same {} can, so add another on type formatting pair for convenience: sometimes it's not that pleasant to write parenthesis in Some(2).map(|i| (i, i+1)) cases and I would prefer r-a to do that for me.

One note: currently,

stdx::never!(snippet_edit.is_none(), "on type formatting shouldn't use structured snippets");
fires always.
Should we remove the assertion entirely now, since apparently things work in release despite that check?

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2023
@DropDemBits
Copy link
Contributor

Should we remove the assertion entirely now, since apparently things work in release despite that check?

Ah, I was the one who added that assert in, and only realized now that this should be a stdx::always instead of a stdx::never 😅

More context on this assert I originally added this assert since structured snippets add in placeholders & tabstops later on instead of embedding them directly, and I didn't think it was worth it right now to port the on-type formatting to structured snippets as it only uses one tabstop in a fixed location.

@SomeoneToIgnore
Copy link
Contributor Author

Thank you, works much better with the different assert 🙂

I've pushed the commit here, but feel free to create a separate PR, you have better context and can describe it better.

@bors
Copy link
Contributor

bors commented Sep 1, 2023

☔ The latest upstream changes (presumably #15542) made this pull request unmergeable. Please resolve the merge conflicts.

@SomeoneToIgnore SomeoneToIgnore force-pushed the more-brackets-on-type-formatting branch from 19a5fa9 to da78617 Compare September 1, 2023 18:49
@Veykril
Copy link
Member

Veykril commented Sep 6, 2023

Now I wonder, we might as well add [ to the mix here no?

@Veykril
Copy link
Member

Veykril commented Sep 6, 2023

Well we can do that in separate PR, but I think that should work in all positions there likewise.
@bors r+

@bors
Copy link
Contributor

bors commented Sep 6, 2023

📌 Commit da78617 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 6, 2023

⌛ Testing commit da78617 with merge 77b359a...

@SomeoneToIgnore
Copy link
Contributor Author

Now I wonder, we might as well add [ to the mix here no?

I personally rarely index something nested with this, so was afraid to add it myself.
Also, was not sure if it's the same as ( rule-wise to apply its counterpart?
Anyway, not sure if very requested feature, so I restrained from bloating the PR.

Ideally, I'd add | since typing closures is sometimes hard, but that needs a but different set of rules.

@Veykril
Copy link
Member

Veykril commented Sep 6, 2023

I was more thinking of arrays than indexing here, unsure whether its useful though.

@bors
Copy link
Contributor

bors commented Sep 6, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 77b359a to master...

@bors bors merged commit 77b359a into rust-lang:master Sep 6, 2023
@SomeoneToIgnore SomeoneToIgnore deleted the more-brackets-on-type-formatting branch September 6, 2023 20:52
@lnicola lnicola changed the title On type format '(', by adding closing ')' automatically feat: On type format '(', by adding closing ')' automatically Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants