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

Spike on auto indentation issues #6943

Closed
luabud opened this issue Aug 12, 2019 · 12 comments
Closed

Spike on auto indentation issues #6943

luabud opened this issue Aug 12, 2019 · 12 comments
Assignees
Labels
area-formatting bug Issue identified by VS Code Team member as probable bug

Comments

@luabud
Copy link
Member

luabud commented Aug 12, 2019

To address #6886 and #6927

At this point only the following things still have to be resolved:

We should look at how Ruby does it in their extension...

[spike conclusion]: #6943 (comment)

@luabud luabud added bug Issue identified by VS Code Team member as probable bug triage-needed Needs assignment to the proper sub-team labels Aug 12, 2019
@kimadeline
Copy link

kimadeline commented Aug 12, 2019

Seems like removing the indentationRules key fixes some of the issues:

indentationRules: {
increaseIndentPattern: INCREASE_INDENT_REGEX,
decreaseIndentPattern: DECREASE_INDENT_REGEX
}

This also means that we should probably expand on the old onEnterRules regexes we had before.


Issues reported by users:

Moving lines up and down:

  • case 1:
# move the next line to the bottom (alt + down arrow)
move_to_end

class Foo:
   def test(self):
      pass

class Bar:
   pass

Pressing return

  • case 1:
def foo():
   pass

# press return here
def bar():
   pass
  • case 2:
class Foo:
    pass
# set the cursor at the beginning of the line and press return

Copy-pasting:

  • case 1:
if 1 == 1:
    print('This line belongs to the if')
newline = 'not part of if' # set the cursor before 'newline' and paste something there
  • case 2:
if 1 == 1:
    print('This line belongs to the if')
newline = 'not part of if' # highlight 'newline' and paste something on it

Another possible broken case:

if True: print('Of course!')
#<--cursor should be here

@DonJayamanne DonJayamanne added area-formatting needs PR and removed triage-needed Needs assignment to the proper sub-team labels Aug 13, 2019
@DonJayamanne DonJayamanne added this to the 2019 - August Sprint 16 milestone Aug 13, 2019
@DonJayamanne
Copy link

TODO:

  • Investigate how indentation works in Ruby
    [ ] Check if there are any issues with their rules

Outcome:

  • If ruby rules work as expected, then we'll adapt their rules into Python extension (Note - separate task/issue/PR)

@jkyeung
Copy link

jkyeung commented Aug 19, 2019

Re: Adapting Ruby indentation behavior for Python

Please take a step back and carefully consider how much we can really borrow from Ruby.

Crucially, Ruby has an end keyword to mark the end of a block (or what is called a "suite" in Python's grammar specs).

Please see my comment on a related issue for an illustration of how the proper dedent level for else cannot be inferred in Python, in general.

If you want to look at existing work to possibly incorporate into this extension, perhaps it should be the highly regarded Python Indent extension.

@ericsnowcurrently ericsnowcurrently self-assigned this Aug 20, 2019
@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Aug 20, 2019

Here are the two regexes from the default VSCode config, expanded verbosely:

increaseIndentPattern
^
\\s*
(
    (
        begin |
        class |
        (
            (
                private |
                protected
            )
            \\s+
            def
        ) |
        def |
        else |
        elsif |
        ensure |
        for |
        if |
        module |
        rescue |
        unless |
        until |
        when |
        while |
        case
    ) |
    (
        [^#]*
        \\s
        do
        \\b
    ) |
    (
        [^#]*
        =
        \\s*
        (
            case |
            if |
            unless
        )
    )
)
\\b
(
    (
        [^#\\{;]
    ) |
    (
        (
            \" |
            ' |
            \/
        )
        .*
        \\4
    )
)*
( # .* )?
$
decreaseIndentPattern
^
\\s*
(
    [}\\]]
    (
        (
            [,)]?
            \\s*
            (
                # |
                $
            )
        ) |
        (
            \\.
            [a-zA-Z_]
            \\w*
            \\b
        )
    )|
    (
        end|rescue|ensure|else|elsif|when
    )\\b
)

Here are the two regexes from the actual Ruby extension (more recent), expanded verbosely:

decreaseIndentPattern
^
(
    (
        \\s*
        (
            module |
            class |
            (
                (
                    (
                        private |
                        protected
                    )
                    \\s+
                )?
                def
            ) |
            unless |
            if |
            else |
            elsif |
            case |
            when |
            begin |
            rescue |
            ensure |
            for |
            while |
            until |
            (
                (?=
                    .*? \\b
                    (
                        do |
                        begin |
                        case |
                        if |
                        unless
                    )
                    \\b
                )
                (
                    (
                        "
                        (
                            \\\\. |
                            [^\\"]
                        )*
                        "
                    ) |
                    (
                        '
                        (
                            \\\\. |
                            [^\\']
                        )*
                        '
                    ) |
                    (
                        [^#"']
                    )
                )*
                (
                    (
                        \\s
                        (
                            do |
                            begin |
                            case
                        )
                    ) |
                    (
                        [-+=&|*/~%^<>~]
                        \\s*
                        (
                            if |
                            unless
                        )
                    )
                )
            )
        )
        \\b
        (?!
            [^;]* ;
            .*? \\b
            end \\b
        )
    ) |
    (
        (
            (
                "
                (
                    \\\\. |
                    [^\\"]
                )*
                "
            ) |
            (
                '
                (
                    \\\\. |
                    [^\\']
                )*
                '
            ) |
            (
                [^#"']
            )
        )*
        (
            (
                \(
                (?!
                    [^\)]*
                    \)
                )
            ) |
            (
                \{
                (?!
                    [^\}]*
                    \}
                )
            ) |
            (
                \[
                (?!
                    [^\]]*
                    \]
                )
            )
        )
    )
).*
$
decreaseIndentPattern
^
\\s*
(
    [}\\])]
    (
        (
            ,? \\s*
            (
                # |
                $
            )
        ) |
        (
            \\.
            [a-zA-Z_]
            \\w*
            \\b
        )
    ) |
    (
        end |
        rescue |
        ensure |
        else |
        elsif |
        when
    )
    \\b
)

@ericsnowcurrently
Copy link
Member

For our "else"/"elif"/"except"/"finally" needs, the focus is on the decreaseIndentPattern.

@brettcannon
Copy link
Member

brettcannon commented Aug 21, 2019

One question I have is if I do:

if a:
    pass
    if b:
        pass
    else:
        pass
        |    # With the cursor here ...

what must I do to get dedenting to work for an else to the top-level if? Do I have to manually dedent once and then let auto-dedent handle the rest? (This is the microsoft/pylance-release#4820 case in this context-insensitive whitespace scenario.)

@brettcannon
Copy link
Member

@jkyeung we can potentially borrow from Ruby thanks to how elsif works as it's similar for at least elif. But the whole point of this spike is to figure out what is feasible and what the ramifications are for the potential solutions.

And the Python Indent extension approach can't be used by us as it overrides the the Enter key which is way to intrusive for us to do.

@jkyeung
Copy link

jkyeung commented Aug 21, 2019

@jkyeung we can potentially borrow from Ruby thanks to how elsif works as it's similar for at least elif.

Well, I guess you could say it's "similar", but then it's just as similar as Ruby's else is to Python's else. My concern remains the same: Ruby has an end keyword to mark the end of what we would call a suite. In Ruby (and pretty much all languages with block-ending delimiters) we can know with 100% certainty where elsif or else should be indented, no matter how badly formatted the code is, as long as it's valid.

# Ruby
if a
puts 'a'
if b
puts 'b'
elsif c  # must go with b
puts 'c'
end
end
# Ruby
if a
puts 'a'
if b
puts 'b'
end
elsif c  # must go with a
puts 'c'
end

Python does not have this property. (Of course, the following code, as-is, isn't even valid, but the point is we can't know for sure which of two valid indentation options is the right one, and if we pick the wrong one, the meaning of the program changes.)

# Python
if a:
print('a')
if b:
print('b')
elif c:  # no way to know
print('c')

But the whole point of this spike is to figure out what is feasible and what the ramifications are for the potential solutions.

I get that. Without understanding those Ruby-handling regexes, I was preemptively guarding against "oh, it works for Ruby, so let's just slap it in for Python!". For all I know, it may be that those regexes happen to be doing what the Python Indent extension says it is doing for else and friends, including NOT dedenting if already dedented. And if that's the case, I think I'm OK with it. But for all I know, it could also be making assumptions that don't hold for Python (such as "else must be dedented exactly one level relative to the previous line, so no matter where the else is now, we will put it there").

I'll hold further comments until more concrete details are presented.

@ericsnowcurrently
Copy link
Member

FWIW, I tried applying just the dedent rules that we reverted and had the exact same problems with inappropriate indent. Presumably the mere presence of indentationRules (or maybe just its "decreaseIndentPattern" property) is enough to trigger the undesirable behavior.

@ericsnowcurrently
Copy link
Member

Given everything I've looked at in the sprint, I don't think it's worth it to pursue this further at the moment. From what I can tell, VS Code make some assumptions about the existence of an end-of-block syntax (which Python almost uniquely does not have for "suites").

So at this point I'm not convinced that any of the existing options provided by VS Code will meet our needs. However, it will be worth bringing this up with the VS Code folks as soon as they have interest/time.

@ghost ghost removed the needs PR label Aug 26, 2019
@brettcannon
Copy link
Member

If we redo our editor.formatOnType to just do whitespace, then we could use the contributes.configurationDefaults setting to turn formatOnType on by default.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-formatting bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

6 participants