Parenthesis wrapping after raise is wrong

[PeterJCLaw]

Environment data

• VS Code version: 1.43.0
• Extension version (available under the Extensions sidebar): 2020.2.64397
• OS and version: Ubuntu Linux 16.04 LTS
• Python version: Python 3
• Type of virtual environment used: n/a
• Jedi or Language Server?: jedi
• Value of the python.languageServer setting: Microsoft

Expected behaviour

Given code like this:

if False:
    raise Thing(|)

Then pressing return with the cursor between the parentheses should wrap the code as so:

if False:
    raise Thing(
        |
    )

This is what you get if either there isn't a raise keyword present or if it is return instead

Actual behaviour

Given code like this:

if False:
    raise Thing(|)

Then pressing return with the cursor between the parentheses actually wraps to:

if False:
    raise Thing(
|)

[karthiknadig]

We have a bigger indentation work item #8996 that covers a lot of issues.

[luabud]

Closing in favour of #8996.

[PeterJCLaw]

Having coincidentally hit this again earlier today, I've just had a look into why this happens. It looks like this is a result of the onEnterRules, specifically the "outdent on enter (block-ending statements)" rule:

vscode-python/src/client/language/languageConfiguration.ts

Lines 77 to 105 in 92e66a0

	// outdent on enter (block-ending statements)
	{
	beforeText: verboseRegExp(`
	^
	(?:
	(?:
	\\s*
	(?:
	pass |
	raise \\s+ [^#\\s] [^#]*
	)
	) |
	(?:
	\\s+
	(?:
	raise |
	break |
	continue
	)
	)
	)
	\\s*
	(?: [#] .* )?
	$
	`),
	action: {
	indentAction: IndentAction.Outdent,
	},
	},

Essentially, the rule is configured to cope with either

if False:
    raise Foo|

or

if False:
    raise Foo(...)|

cases, but not the case reported above:

if False:
    raise Foo(|)

Personally I would happily trade off having the raise Foo| and raise Foo()| cases outdent in order to make the raise Foo(|) case work. In my experience it's easier and more convenient to delete additional whitespace than it is to need to manually add it.
In this case the effort needed to fix the errant layout the editor has created is considerably larger than the single removal it trades off for, which makes this more compelling to me.

I realise that #8996 exists with a goal of a more complete solution, however that's been the case for a considerable time without any movement and it sounds like the approach being explored there is a much more complete auto-formatter which is likely to have side effects beyond fixing this sort of cursor-positioning/indent issue and may put off people (myself included) who do not like auto-formatters.

@luabud would you consider a PR to flip the trade-off as I've described above?
I strongly suspect we could mitigate the majority of effect on the other side of the trade-off by explicitly picking up, say, two levels of nested parentheses, so that simple examples (raise Foo|, raise Foo()| and even raise Foo("{}".format(...))) do continue to work as they do now.

[PeterJCLaw]

Cross linking: looks like #10285 is another instance of exactly this case.

[luabud]

@PeterJCLaw, about this:

my experience it's easier and more convenient to delete additional whitespace than it is to need to manually add it.

My concern is that a lot of people may not feel the same way. I personally prefer to add spaces than deleting it (I guess it's mostly because that's what I got used to do 😅) . I wouldn't be reluctant to accept this change if it only impacts raise though, and not continue nor break.

In any case I'm reopening this and marking as "needs decision" so I can talk to the team about this 😊

[PeterJCLaw]

Thanks!

One additional data-point I've spotted now that I've been thinking about this is that return seems already to be making the trade-off that I'm advocating for here:

if False:
    return foo(|)

becomes

if False:
    return foo(
        |
    )

and

if False:
    return foo|

becomes

if False:
    return foo
    |

I agree that the current behaviour makes sense for break and continue -- they can't be followed by a call (or indeed anything), so they'd not hit this issue.

[luabud]

@PeterJCLaw I talked to the team and everyone was ok with a PR for the change for raise 😊

[PeterJCLaw]

@luabud thanks! I've raised #15516, which I've manually checked has the expected behaviour. I've gone with keeping this really simple for now (i.e: not attempt to handle raise Foo()| specially), but I'm happy to explore that if you feel it's needed.