-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Auto indent on insert_at_line_start
#5837
Auto indent on insert_at_line_start
#5837
Conversation
I haven't looked too closely at the implementation yet but this does seem very useful to me and like a better default too. Thanks for working on this! That being said we might want to keep the old behaviour as a seperate command so people that don't like this change can rebind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is good I think, the implementation can be simplified (and is not quite correct, see review comments)
Thanks for the detailed review and pointers @pascalkuthe. I expected that I was doing a lot of these things sub-optimally. I'll try to address these issues soon. |
6f45df3
to
a5c54b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor nits otherwise the implementation now LGTM.
a5c54b2
to
31ab06f
Compare
I made the last suggested changes, thanks again. I also created a second commit that adds the same behavior to |
cc7fd66
to
21136b8
Compare
Should I look into creating a test for the new behavior? |
I believe there is currently no tests for the existing behaviour so I wouldn't consider it a blocking issue but more tests are always good so if you want to add them go ahead |
I added a test and found one remaining issue (which I fixed). The test is a little ugly but it's the best I could do 👀 fn foo() {
if let Some(_) = None {
}
}
fn bar() {
} (there's one line with existing whitespace) Is this okay or should I change the test? |
Looks like some recent changes are causing conflicts. Let me know when somebody wants to review this and I'll resolve them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been testing this for a bit and did a pretty thorough review a while ago. Once the bugfix release (23.3.1) is done we can resolve the conflicts here (should be pretty easy) I think we can land this
e4f9138
to
e772d1b
Compare
Sorry for the long delay. I rebased onto latest master, should be good to go now. |
e772d1b
to
75a2fa4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a small style nit but otherwise I think this looks great (especially the integration tests!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for working on this! This is a great QOL improvement
First attempt at #4859. This makes
I
andA
automatically indent, but only if the line is empty. Otherwise it behaves exactly as before. I thought about also handling indentation when the line only contains white space, but I left this out for now. Either way, this is a breaking change.I roughly followed the implementation of
open_above
/open_below
. Though, I'm not sure if the implementation is very good, all the text editing functions in the code base are new to me.Here's the result of
insert_at_line_start
/I
with this change (the only difference is on the empty line):I tried to handle all edge cases (i.e. cursor is at document start/end, multiple selections) and so far it seems to work.
I also plan to make.insert_at_line_end
behave similarlyEdit:
insert_at_line_end
now works the same.closes #4859