Fix removing of extra lines on the edge of enabled lines boundaries.

[kevincox]

This was broken by dd9f72b#diff-1020ef5a942231c7da662fa974a70a3bR145 which added a hacky fix that prevented removing newlines.

I haven't fixed the test that was added in that commit because I haven't yet work out how to tell if a range is affected by a disable comment. The correct solution will be to handle that comment instead of disallowing removal of lines.

Help welcome.

[kevincox]

cc @gwelymernans You introduced this issue.

[bwendling]

Could you also update the CHANGELOG.

[kevincox]

Could you also update the CHANGELOG.

Done.

[bwendling]

Great! Once you're ready, go ahead and send the PR.

[kevincox]

I'm having trouble figuring out when lines are disabled via a comment in the code. It doesn't seem like they are added or removed from lines so there must be some other mechanism in play.

[bwendling]

After the program is parsed and the unwrapped lines are created, it goes through to determine if there are any disable comments. If so it marks the affected lines as disabled.

[kevincox]

Ok, I have fixed the disable check. It is a little hacky but better then before.
What is does is if the current line and the previous like are disabled it removes the lines from the set of editable lines.

A better approach would be to remove disabled lines from the set of editable lines for the whole reformat but that would require more refactoring and would best be done in a different fix.

[coveralls]

Coverage increased (+0.003%) to 95.589% when pulling 0bd2d55 on kevincox:remove-edge into f9a3b35 on google:master.

[coveralls]

Coverage increased (+0.7%) to 95.597% when pulling fb0220e on kevincox:remove-edge into b65cd13 on google:master.

[coveralls]

Coverage increased (+0.7%) to 95.597% when pulling fb0220e on kevincox:remove-edge into b65cd13 on google:master.

[bwendling]

Ok, I have fixed the disable check. It is a little hacky but better then before.
What is does is if the current line and the previous like are disabled it removes the lines from the set of editable lines.

A better approach would be to remove disabled lines from the set of editable lines for the whole reformat but that would require more refactoring and would best be done in a different fix.

+1

Could you add some tests to yapf_test.py? That way you can use the --lines=<lines> option. Also a few to reformatter_basic_test.py for coverage. I want to make sure this doesn't regress. (I should have mentioned this before. Sorry about that.)

[kevincox]

On Thu., Mar. 21, 2019, 23:20 Bill Wendling, ***@***.***> wrote: Ok, I have fixed the disable check. It is a little hacky but better then before. What is does is if the current line and the previous like are disabled it removes the lines from the set of editable lines. A better approach would be to remove disabled lines from the set of editable lines for the whole reformat but that would require more refactoring and would best be done in a different fix. +1 Could you add some tests to yapf_test.py? That way you can use the --lines=<lines> option. Also a few to reformatter_basic_test.py for coverage. I want to make sure this doesn't regress. (I should have mentioned this before. Sorry about that.)

What specifically would you like tested? I have added some tests and there are tests for the disable case. Do you just want more variety of the same sort of tests?

…

[bwendling]

Just some of the examples that demonstrated this bug. The blank lines calculator tests are good, but it doesn't necessarily indicate that the reformatter won't still mess things up.

[kevincox]

Between the existing cases in blank-lines and the test added in dd9f72b the obvious cases are covered. I will see if I can think up a couple more cases when I get the chance.

[bwendling]

Okay. We can merge this then. If you can't think of others then no worries...