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

Add line missing line ending. #6

Merged
merged 1 commit into from Aug 31, 2015
Merged

Conversation

davidolrik
Copy link
Contributor

When reassembling the lines, no line ending were added to the last line.
This caused the following line to be pulled up unto the last line of the selected block.

@alimony
Copy link
Owner

alimony commented Aug 30, 2015

I cannot reproduce this, could you give me an example of a block of lines and what part to select before doing the sort? I could only make it happen by selecting the lines I wanted to sort and also including the last line break, i.e. selecting up until the first position of the next not selected line, if that makes any sense.

@davidolrik
Copy link
Contributor Author

If I select a block of lines and sort, as shown in the first image, the last line is moved up.
Doing the same, with the build in sorting functions does not move the last line up.

napkin 31-08-15 9 56 37 am

@davidolrik
Copy link
Contributor Author

Your explanation makes total sense, and that is usually how I select lines to sort.
I position the cursor on the first line I want to select, and then do ⇧🔽 until I have select all the lines I want.

@alimony
Copy link
Owner

alimony commented Aug 31, 2015

Sounds entirely reasonable to mimic the built-in sort behavior, so this change should look if the last character is a line ending and only add one if there was one before. Otherwise it would add an unwanted line ending if the selection is up to the last character of the last line instead.

@davidolrik
Copy link
Contributor Author

Agreed, I have updated my pull request to respect the last character(s) of the region.

@alimony
Copy link
Owner

alimony commented Aug 31, 2015

I think it can be much simpler, something like:

if self.view.substr(region).endswith(line_ending_character):
    output += line_ending_character

I hope you are not annoyed by my suggestions :)

@alimony
Copy link
Owner

alimony commented Aug 31, 2015

Also reminds me that I should really add some tests…

When reassembling the lines, no line ending were added to the last line, even if the selection ended with a line ending character.
This caused the following line to be pulled up unto the last line of the selected block.
@davidolrik
Copy link
Contributor Author

Not annoyed at all 😄
I'm all for deleting code, and keeping it simple, I have updated my pull request with your suggestion.
Works as expected.

@alimony
Copy link
Owner

alimony commented Aug 31, 2015

Great, this is perfect. Will merge it as soon as I'm on my main computer and can test it one last time myself.

@alimony
Copy link
Owner

alimony commented Aug 31, 2015

Haha, look what Gmail did to that smiley in the notification email:

skarmavbild 2015-08-31 kl 14 02 33

That is one messed up guy!

@davidolrik
Copy link
Contributor Author

Jabba the smiley! (...or more likely his sister)

alimony pushed a commit that referenced this pull request Aug 31, 2015
Add line missing line ending.
@alimony alimony merged commit 45a85ac into alimony:master Aug 31, 2015
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.

None yet

2 participants