Skip to content

feat: join lines joins two ifs into else if #9473

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
Jul 3, 2021

Conversation

matklad
Copy link
Member

@matklad matklad commented Jul 3, 2021

bors r+
🤖

join-if

@bors
Copy link
Contributor

bors bot commented Jul 3, 2021

@bors bors bot merged commit 8d0fa9c into rust-lang:master Jul 3, 2021
@jhpratt
Copy link
Member

jhpratt commented Jul 5, 2021

Is there any other place joining lines has behavior other than just joining the lines? This seems quite counterintuitive to me.

@flodiebold
Copy link
Member

It already removes braces and commas. If you just want to join lines, you can use the editor's built-in functionality; the whole point of this function is to be smarter. And there's really no good reason to join } and if { into } if {, unless you want } else if {. I think there are probably a lot of other similar opportunities to DWIM.

@jhpratt
Copy link
Member

jhpratt commented Jul 5, 2021

That's just syntactic, not semantic, setting aside macros for obvious reasons. Imo "no other reason you'd do that" is quite dangerous when I join lines. Personally, I overrode the editor's built-in join lines to be r-a because I expect it to be a drop-in replacement.

@flodiebold
Copy link
Member

Undo is just a keypress away. Still, I don't see any situation where you'd join these lines without intending this change. But I'm sure @matklad would be open to making this configurable ;)

@jhpratt
Copy link
Member

jhpratt commented Jul 5, 2021

It's more the unexpectedness factor for me. I don't think that "join lines" should do anything but save for some syntactic cleanup. I have no issue waiting for a response, though 🙂

@matklad
Copy link
Member Author

matklad commented Jul 5, 2021

Yeah, adding a config flag should be fine. I guess, I probably should heed my own advice and proactively add flags for other bits of functionality here :)

Couple of more things to do:

  • document special cases that join lines has
  • consider renaming it to just "join"?

@matklad matklad deleted the join-lines branch July 5, 2021 11:01
@rogerdahl
Copy link

At first glance, this looks like an automated refactoring which shouldn't change semantics, but if the two predicates are not mutually exclusive, or if there are side effects, it will. So it seems like it could be a footgun for the unwary... Is there a way to tell if something is an automated refactoring or a code editing shortcut?

@matklad
Copy link
Member Author

matklad commented Jul 5, 2021

@rogerdahl explicitly invoked actions are "do what I mean" rather than "do not change semantics". If the user invokes join lines (which is an explicit shortcut) between two ifs, that's a signal for the IDE that they want to join the two lines.

@matklad
Copy link
Member Author

matklad commented Jul 5, 2021

Note that this behavior is now configurable: https://github.com/rust-analyzer/rust-analyzer/pull/9503/files#diff-00784dde03b1be151004f435a4ca74754cb674a9e789840e12760d8f3ac1728fR727

@jhpratt
Copy link
Member

jhpratt commented Jul 5, 2021

Thank you!

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.

4 participants