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.

Sign up for GitHub

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

Jump to bottom

Determine whether to use a margin of 0 or 1 when uncommenting #476

Merged
merged 12 commits into from
Jul 26, 2021
Merged

Determine whether to use a margin of 0 or 1 when uncommenting #476

merged 12 commits into from
Jul 26, 2021

Conversation

Omnikar
Copy link
Contributor

@Omnikar Omnikar commented Jul 21, 2021 •
edited
Loading

Fix #474. This change will cause uncommenting to use a margin of 0 when any of the uncommented lines does not have a space after the comment token, rather than unconditionally using a margin of 1. I apologise if I might have overlooked something, I've never actually contributed to an open source project before.

@cessen
Copy link
Contributor

cessen commented Jul 21, 2021

This looks mostly good to me. But a couple of things:

  1. It would probably be better to roll the check into find_line_comment(), and have it returned as another element in the tuple.
  2. There appears to be another bug (that was already there, not in your code) where lines that have multiple selections on them get multi-commented. For example, if a line has two selections, then it will be prefixed with // // instead of //. I don't know if you want to tackle that as well, but if you're up for it then we might as well roll it into the same PR. Totally up to you.

pickfire
pickfire reviewed Jul 21, 2021
View reviewed changes
helix-core/src/comment.rs Outdated Show resolved Hide resolved
@Omnikar
Copy link
Contributor Author

Omnikar commented Jul 21, 2021 •
edited
Loading

  1. It would probably be better to roll the check into find_line_comment(), and have it returned as another element in the tuple.

@cessen Ok, this is done in 6055e93. I'll see if I can figure out #​2.

@Omnikar
Copy link
Contributor Author

Omnikar commented Jul 21, 2021

  1. There appears to be another bug (that was already there, not in your code) where lines that have multiple selections on them get multi-commented. For example, if a line has two selections, then it will be prefixed with // // instead of //. I don't know if you want to tackle that as well, but if you're up for it then we might as well roll it into the same PR. Totally up to you.

Fixed in a7dd731.

pickfire
pickfire reviewed Jul 21, 2021
View reviewed changes
helix-core/src/comment.rs Outdated Show resolved Hide resolved
@Omnikar
Fix find_line_comment test for new return type
4a2ae1d
@Omnikar
Generate a single vec of lines for comment toggle
96b6233 
`toggle_line_comments` collects the lines covered by all selections into
a `Vec`, skipping duplicates. `find_line_comment` now returns the lines
to operate on, instead of returning the lines to skip.
@Omnikar
Fix test for find_line_comment
1032a4b
@Omnikar
Reserve length of to_change instead of lines
cc9344a 
The length of `lines` includes blank lines which will be skipped, and as
such do not need space for a change reserved for them. `to_change`
includes only the lines which will be changed.
@Omnikar Omnikar requested a review from pickfire July 21, 2021 22:26
pickfire
pickfire reviewed Jul 22, 2021
View reviewed changes
helix-core/src/comment.rs Outdated Show resolved Hide resolved
pickfire
pickfire approved these changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@pickfire pickfire left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside the change mentioned looks good to me. Not tested.

Thanks for working on this. :D

archseer
archseer requested changes Jul 22, 2021
View reviewed changes
helix-core/src/comment.rs Outdated Show resolved Hide resolved
helix-core/src/comment.rs Show resolved Hide resolved
pickfire
pickfire approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me besides on the comment that it should have. Not tested for a second time.

@Omnikar Omnikar requested a review from archseer July 24, 2021 23:36
@Omnikar Omnikar closed this Jul 25, 2021
@Omnikar Omnikar deleted the branch helix-editor:master July 25, 2021 05:54
@Omnikar Omnikar deleted the master branch July 25, 2021 05:54
@Omnikar Omnikar restored the master branch July 25, 2021 05:56
@Omnikar Omnikar reopened this Jul 25, 2021
@Omnikar
Copy link
Contributor Author

Omnikar commented Jul 25, 2021

I deleted the master branch of my fork accidentally, sorry.

@archseer archseer merged commit 112ae5c into helix-editor:master Jul 26, 2021
@Omnikar Omnikar deleted the master branch July 26, 2021 02:09
@Omnikar Omnikar restored the master branch July 26, 2021 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cessen cessen cessen left review comments

@pickfire pickfire pickfire approved these changes

@archseer archseer Awaiting requested review from archseer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Uncommenting lines that are commented without a space deletes an extra character
4 participants
@Omnikar @cessen @archseer @pickfire