PR: Fix commenting/uncommenting to not change leading whitespaces

[remisalmon]

Description of Changes

• Wrote at least one-line docstrings (for any new functions)
• Added unit test(s) covering the changes (if testable)

• Included a screenshot or animation (if affecting the UI, see Licecap)

This PR changes the way spyder uncomments code and fixes an issue where toggling comments on and off over lines with an uneven number of leading whitespaces would add an extra whitespace (regardless of indentation level).
Example:

from pprint import pprint
x = [1,
     2]
y=[1,
   2]
def f(x, y):
    pprint(x,
           y)

Commenting (Ctrl+1) then uncommenting (Ctrl+1), before:

from pprint import pprint
x = [1,
      2]
y=[1,
    2]
def f(x, y):
    pprint(x,
            y)

Commenting (Ctrl+1) then uncommenting (Ctrl+1), after:

from pprint import pprint
x = [1,
     2]
y=[1,
   2]
def f(x, y):
    pprint(x,
           y)

This PR breaks compatibility with spyder 3: commenting with spyder 3 and uncommenting with spyder 4 with break indentation but that should not be happening much.

This PR also break toggling comments on manually commented lines (prefixing '#' instead of '# ') but that should be expected and other IDE behave the same (at least VSCode and Atom do).

Issue(s) Resolved

Fixes #14326

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: remisalmon

[dalthviz]

Hi @remisalmon thanks for the help! However after a quick review seems like you changed the previous test available. So the new version of the tests seems like are not covering all the cases that the old test did?

Pinging @ccordoba12 just in case

[remisalmon]

@dalthviz yes I did changes the tests because the changes to __remove_prefix() to fix the issue I described do not pass the current tests - which are design to keep compatibility with spyder 3-style comments (where the prefix is '#' instead of '# ').

So for ex. toggling the comments off (Ctrl+1) on spyder 3-style comments like:

#from pprint import pprint
#x = [1,
#     2]
#y=[1,
#   2]
#def f(x, y):
#    pprint(x,
#           y)

Results in:

from pprint import pprint
x = [1,
    2]
y=[1,
  2]
def f(x, y):
   pprint(x,
          y)

Which breaks the indentation but should be expected and is the default behavior in other editors (tested with VSCode and Atom). Hope that makes sense!

[dalthviz]

Thanks for the explanation @remisalmon 👍

What do you think @ccordoba12, @spyder-ide/core-developers ?

[impact27]

I think it makes sense that commenting / uncommenting should not change the file. I think the change as described here looks reasonable.

[bcolsen]

While I agree that it is very unfortunate that the change in code formatting, as describes #14326, occurs, it seems unfortunate to throw out all the compatibility with manual and Spyder 3 comments that we currently have.

This change goes from breaking formatting, to breaking code (indents). The code above may now get a difficult to find indentation error with the def block now at three spaces,

It seems that Spyder is alone in caring about this however as VSCode, Atom and even Pycharm and Jupyter-lab prefer this simpler approach. It's does have an advantage from a maintainability perspective but we are losing some user friendliness by making code errors.

[impact27]

Well now if I uncomment this code:

#def a():
# def a():
#     return
#    return

The return statement is correctly uncommented to 4 spaces, even if one line has 4 spaces and one line has 5.
So what about: remove one space unless it breaks indentation?
An alternative would be: if uncommmenting a block, the context can be extracted from the previous line:

#x = [1,
#     2]

vs

# x = [1,
#      2]

The number of spaces before the x may be used? Otherwise we can use the current method.

[bcolsen]

I believe currently the code will remove one space only if the number of spaces between the # and the code is odd. This works pretty well for 2 space and 4 space indents.

So what about: remove one space unless it breaks indentation?

Yes that would be the goal, but how can we tell if it would break the indentation?

An alternative would be: if uncommmenting a block, the context can be extracted from the previous line:

I like this. If a block is continuing to multiple lines remove the same number of spaces from all the line of the block as the block starting line.

The code would also have to detect function call and definition headings continuations. Currently this is a problem as well:

# def ab(a=1,
#        b=2):
#     return

[impact27]

Yes that would be the goal, but how can we tell if it would break the indentation?

fix_indent_smart kind of does that. I am actually changing this function a bit in #14467 . I separate this code in two parts: find_indentation That gives you the indentation for the current line, extracted from the previous lines, and fix_indent_smart that uses this information to fix the indentation. Using these, we could decide how many spaces to remove based on the indentation from the previous uncommented lines, uncommmenting one line at a time.

The code would also have to detect function call and definition headings continuations. Currently this is a problem as well:

Alternatively, we keep the current behaviour, unless multiple lines are deindented, one of them has only a single space after the #, and none of them have zero spaces.

[bcolsen]

find_indentation is cool. It has all the things that need to happen here. Does it only find one level for the whole block? So it wouldn't work with mixed comments(space and no space...Who would do such a thing?...I would ;-) ).

I looks like it has a bracket matching element in there with self.__get_brackets that might be all we really need to keep the code the same and just make sure we apply the right spacing in the bracket blocks.

[remisalmon]

I rebased this on 5.x, this is still breaking compatibility with Spyder 3 which is still better than breaking formatting after uncommenting commented code, in my opinion.

[dalthviz]

Hi @remisalmon thank you for checking this again! I think due to this breaking compatibility with Spyder 3 (and also since we are not planning to do further Spyder 5 release for the moment) it needs to be rebased on top of the master branch. But what do you think @spyder-ide/core-developers ?

[ccordoba12]

Hey @remisalmon, thanks for your work and sorry for not reviewing it before.

I like that your changes made our code much simpler but I'm concerned about the changes you did to our tests (see below).

[ccordoba12]

Why is this space not present now? I think your changes shouldn't alter this, so I don't understand this change.

The same goes for all modifications you did for all other tests here.

[remisalmon]

The text line 35 is defined with Spyder 3 style comments (leading # with no space) but the tests are expecting Spyder 4/5/6 style comments (# with a space).

This PR breaks compatibility with Spyder 3 style comments which I have argued elsewhere in the comments is prefered (in my opinion) to breaking the formatting after commenting/uncommenting. In this case we could rewrite the tests to use Spyder 4/5/6 style comments.

[ccordoba12]

To move your work to the master branch, please run the following commands:

git checkout master
git checkout fix_uncomments
git rebase --onto master 5.x fix_uncomments
git push -f origin fix_uncomments

[remisalmon]

Thanks @ccordoba12 for taking a look, I rebased on the master branch and replied to your comment.

(I also tried to look at the find_indentation, get_line_indentation, fix_indent_smart, fix_indent, simple_indentation etc. helper functions but my head started to hurt)

[ccordoba12]

Thanks @ccordoba12 for taking a look, I rebased on the master branch and replied to your comment.

Thanks @remisalmon! I tested your changes locally and they are working as expected, so thanks a lot for your work and your patience with us.

I think the new way of toggling comments feels natural and intuitive, so I really like it. I only have one question about a sentence in your OP:

This PR also break toggling comments on manually commented lines (prefixing '#' instead of '# ') but that should be expected and other IDE behave the same (at least VSCode and Atom do).

Could you post an example about this new behavior to understand better what you mean?

[remisalmon]

Thanks @ccordoba12 for taking a look, I rebased on the master branch and replied to your comment.

Thanks @remisalmon! I tested your changes locally and they are working as expected, so thanks a lot for your work and your patience with us.

🎉

I think the new way of toggling comments feels natural and intuitive, so I really like it. I only have one question about a sentence in your OP:

This PR also break toggling comments on manually commented lines (prefixing '#' instead of '# ') but that should be expected and other IDE behave the same (at least VSCode and Atom do).

Could you post an example about this new behavior to understand better what you mean?

Take this code and uncomment the commented lines (first two use the old Spyder 3 comment style, last two use the new Spyder 4/5/6 comment syle):

if True:
#    pass
    #pass
#     pass
    # pass

Spyder + this PR:

VSCode:

Atom/Pulsar:

Spyder currently:

(in fact this PR only breaks compatibility with the old Spyder 3 comment style in 1 of the 2 cases)

[ccordoba12]

Ok, thanks for the clarification. Since this makes Spyder behave as other IDEs in that particular case, I agree with you that it's the right thing to do.

[ccordoba12]

(I also tried to look at the find_indentation, get_line_indentation, fix_indent_smart, fix_indent, simple_indentation etc. helper functions but my head started to hurt)

Missed this one, lol! Yeah, that code is not easy to follow but we'd really appreciate your help to improve it later if you have the energy for it.

[ccordoba12]

Thanks @remisalmon!