-
Notifications
You must be signed in to change notification settings - Fork 179
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
Preserve spacing of toplevel comments #1554
Conversation
d1b60d0
to
1c58508
Compare
Waiting for #1549 |
1c58508
to
1a0e27d
Compare
730bc58
to
7531340
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.
Looks good.
@@ -180,6 +180,8 @@ module Location = struct | |||
|
|||
let compare_end_col x y = Position.compare_col x.loc_end y.loc_end | |||
|
|||
let line_difference fst snd = snd.loc_start.pos_lnum - fst.loc_end.pos_lnum |
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.
This function doesn't make sense if fst
is after (both start and end positions) snd
. I think we should at least test assert (fst.loc_start.pos_lnum <= snd.loc_start.pos_lnum)
.
Also I'm not sure what this function means if snd
is inside fst
, we should probably assert that the result is >= 0
too.
Could you add some documentation about that ?
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 agree on the principle, but I've just discovered that this invariant doesn't hold anymore when comments have been relocated, so for now I've just added a docstring for this function and I will investigate more deeply this relocation issue in another PR. It doesn't look blocking for this PR as this is what was already done in master (the difference computation) anyway.
4d7209c
to
7a74ccd
Compare
7a74ccd
to
8c52ad0
Compare
Motivation:
was formatted as:
because the comment was wrongfully attached to the second item instead of the first one.
edit: no diff with test_branch.sh after the unnecessary commits were dropped.