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

Allow more powerful onEnterRules for cursor alignment #66235

Open
jakebailey opened this issue Jan 8, 2019 · 16 comments
Open

Allow more powerful onEnterRules for cursor alignment #66235

jakebailey opened this issue Jan 8, 2019 · 16 comments
Assignees
Labels
editor-autoindent Editor auto indentation issues editor-input Editor text input feature-request Request for new features or functionality
Milestone

Comments

@jakebailey
Copy link
Member

In the Python extension, we'd like to add support for variable-length indents on enter to better support PEP8 formatting (microsoft/vscode-python#481 and others). For example:

def func(a,
         |

Hitting enter on the first line should stick the cursor where | is, not just a single indent. In this second case, it should do a single indent:

def func(
    |

There appears to only be two ways to handle this:

  • onEnterRules
  • On-type formatting

We'd like to avoid the latter, as we currently support both Jedi and our own language server, which means implementing on-type formatting in two different places (since a single language cannot have multiple on-type formatting providers). Plus, on-type formatting is not enabled by default nor is very popular (and enables a slew of formatting rules other than cursor alignment), so forcing users to enable on-type formatting as a whole would be good to avoid.

The other candidate is onEnterRules, but it's not powerful enough to be able to do something like this, as:

  • The "actions" work in terms of indents, whereas these cursor positions may not be a round multiple of an indent, meaning appendText would be required, but...
  • appendText is only allowed to be a static string, and cannot be generated on the fly.
  • Rules are applied using regex to deduce context, which gets unweildy, and even more difficult without onEnterRules should be multi-line aware #50952.
  • Even with something like Add support for capture groups in onEnterRules #17281, there wouldn't be a way to convert a capture group to "whitespace of the same length of the capture group" (plus the issue of tab indents).

My initial thought was to allow a function to be provided in onEnterRules instead of just specifying appendText. For example, something like:

{
    beforeText: /\s+def\s+\w+\(\w+/,  /* not the real regex */
    action: {
        indentAction: IndentAction.None,
        appendText: (document, position, formatOptions, ... /* something */) => calculateIndent(...) // returns a string
    }
}

However, on closer inspection of how onEnterRules works, it appears that it needs to be serializable, so I'm not sure it'd be able to have a callback like the above.

Is there something that can be added to allow more powerful rules on-enter? Or, is there something that already exists that we've missed?

@vscodebot
Copy link

vscodebot bot commented Jan 8, 2019

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

@vscodebot vscodebot bot added the editor-autoindent Editor auto indentation issues label Jan 8, 2019
@jakebailey
Copy link
Member Author

This thread is related (I missed this thread when looking around): #3088 (comment)

@RMacfarlane RMacfarlane added the feature-request Request for new features or functionality label Jan 8, 2019
@alexdima
Copy link
Member

alexdima commented Jan 9, 2019

fyi @jrieken

Yes, onEnterRules are evaluated on the renderer process where we don't execute any foreign (extension) code. So whatever we come up with must be descriptive (i.e. non-imperative).

I think #17281 could be a promising solution for this use case. With a lot of inspiration from snippets, I imagine the following rule could cover the above:


def func(a,
         |
{
  beforeText: /^\s*(def \w+\().*,$/,
  action: { indentAction: IndentAction.None, appendText: '${1/./ /g}' }
}

${1/./ /g} would mean use the capture group 1 from the regex (def func() and replace everything with spaces, so appendText would be 9 spaces.


@jakebailey
Copy link
Member Author

That's definitely a compelling solution; I wasn't aware of that snippet grammar.

One concern I had with anything using appendText would be users who are using tab-indented Python (which, technically speaking, would mean that they're not following PEP8 and the extra indention wouldn't make sense). But I think that the rule could become something like:

{
  beforeText: /^\s*(def \w+\().*,$/,
  action: { indentAction: IndentAction.None, appendText: '${1/[^\t]/ /g}' }
}

And be able to mostly do the right thing, though we'd have to figure out how to ensure hitting enter doesn't insert something that makes the Python interpreter fail. (That's part of the reason I included formatOptions in my example rule modification, so that you could make decisions based on tab sizes.)

@jrieken
Copy link
Member

jrieken commented Jan 10, 2019

And be able to mostly do the right thing,

That's why I am very much in favour of the format-on-type-solution. @jakebailey Sure, it's called format-on-type but no rule exist that it must do the full formatting cycle, e.g. for a start you could implement (one shared) formatter that does exactly (and only) what is desired here.

which means implementing on-type formatting in two different places (since a single language cannot have multiple on-type formatting providers).

Maybe you can implement it once, in the TS portion of your extension. The provider knows for what character and position it is invoked.

Plus, on-type formatting is not enabled by default nor is very popular (and enables a slew of formatting rules other than cursor alignment)

What the formatter does it up to your implementation and your extension is also free to enable format-on-type for it self.

@DonJayamanne
Copy link
Contributor

Maybe you can implement it once, in the TS portion of your extension. The provider knows for what character and position it is invoked.

Thanks, but we will only need this for the language server (one place). @jakebailey /cc

What the formatter does it up to your implementation and your extension is also free to enable format-on-type for it self.

@jrieken
Q. How can the language server do this without the user having editor.formatOnType enabled?
I.e. how can language server implement indentation formatting as the user types, regardless.

@MikhailArkhipov
Copy link

MikhailArkhipov commented Jan 10, 2019

Could it be done same way on type formatting works? I.e. ask for text edits on enter and apply the returned whitespace indent? We would prefer to keep on-type formatting separate from indentation, like VS IDE does. Many people like smart indentation but do not enable on-type formatter. Also, having full information and AST makes it easier and faster to calculate the indentation, as well as easier to debug as compared to regex. The AST is in C# in the language server.

@jrieken
Copy link
Member

jrieken commented Jan 11, 2019

Q. How can the language server do this without the user having editor.formatOnType enabled?

Yeah, you would need to change the setting on behalf on the user (for that language). We have API for this and I know that some extensions do these things.

@jrieken
Copy link
Member

jrieken commented Jan 11, 2019

Could it be done same way on type formatting works? I.e. ask for text edits on enter and apply the returned whitespace indent?

Something like an indentation provider? That's something we have briefly talked about and this is obviously the most powerful solution. The challenge is that it makes pressing Enter async, e.g we need to check the with the extension host process first, and that means pressing Enter might feels sticky (when the extension is slow or blocked)

@DonJayamanne
Copy link
Contributor

Yeah, you would need to change the setting on behalf on the user (for that language). We have API for this and I know that some extensions do these things.

@jrieken Please could you point us in this direction.
Just to be clear, what we don't want is:

  • A separate user defined flag to enable/disable indentation formatting and format on type formatting.
  • We'd like this indentation provider enabled at all times.
  • It's my understanding that we cannot implement an indentation provider based on format on type without relying on the formatOnType setting.

@MikhailArkhipov
Copy link

@jrieken 'Something like an indentation provider?' - yes, exactly. It does can feel sticky if extension is badly written :-) (happens in VS IDE). But generally it is up to the extension to decide and deal with how to do it fast, IMO.

@kbrose
Copy link

kbrose commented Mar 20, 2019

Hi, can we get an update on this? Is this actively being worked on or currently somewhere in the backlog?

@ericsnowcurrently
Copy link
Member

@alexandrudima, @jakebailey, something along the lines of #66235 (comment) should work, but it will not meet all our needs without multi-line regex support (#50952). Dealing with regexes is a pain (as opposed to AST a la the language server), but it seems more feasible.

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Jun 11, 2019

One alternative, perhaps more appropriate, is to change LanguageConfiguration.brackets from CharacterPair[] to BracketRule[] (or (CharacterPair | BracketRule)[] for backward compatibility).

The new types would looks like this:

interface BracketRule {
    brackets: CharacterPair;
    onEnterAction?: BracketEnterAction[];
}

interface BracketEnterAction {
    afterText?: RegExp;
    indentAction: IndentAction.None | IndentAction.Indent | IndentAction.Continue;
    indentActionClose?: IndentAction.None | IndentAction.Indent | IndentAction.Continue;
}

The current default appears to be:

{  // BracketEnterAction
    indentAction: IndentAction.Indent,
    indentActionClose: IndentAction.None
}

(This could also help with #67678.)

@brettcannon
Copy link
Member

brettcannon commented Jun 26, 2019

Another reason we are after more powerful rules:

if a:
    print("a")
    else:   # Should be dedented after pressing Enter.

With the current IndentAction there is no way to provide a rule to say "the current line should be indented/dedented". It seems most other languages rely on bracket pairing to have VS Code automatically handle this sort of thing as they typically have e.g. } or end while Python only has whitespace.

Even extending IndentAction so we can control what the indentation of the current line instead of the following line would be helpful to at least start addressing our indentation issues (although it won't solve our top most requested issue to fix that Jake outlined to start off this issue, and it is our top one by a large margin).

@kbrose
Copy link

kbrose commented Jun 26, 2019

Yep, that's a good feature. Just wanted to add on a complex interaction I had to solve for in my extension recently, in case someone uses this thread as a place to find requirements :)

if a:
    return a  # next line should be dedented

def f(a):
    if a:
        return a  # next line should be dedented
    else:  # since this line was already dedented because of the return, do NOT dedent on Enter
        return not a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor-autoindent Editor auto indentation issues editor-input Editor text input feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

10 participants