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

Remove patterns for decreasing next indent level #212

Merged
merged 1 commit into from Oct 17, 2017

Conversation

Projects
None yet
2 participants
@50Wliu
Member

50Wliu commented Oct 17, 2017

This PR reverts the functional changes of #204. Unfortunately, there is no way (yet) to tell if we're in an if block, which can lead to multiple dedenting in the following scenario:

  if True:
    # something
    return
  # we are now at this indent level because of `decreaseNextIndentPattern`
else: # but typing `else:` will trigger another dedent from `decreaseIndentPattern`

I would argue that that is worse than having to shift-tab after a return that is not in an if statement.

/cc @kbrose

Fixes #209

@50Wliu 50Wliu merged commit 98547ff into master Oct 17, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@50Wliu 50Wliu deleted the wl-revert-decrease-next-indent branch Oct 17, 2017

@kbrose

This comment has been minimized.

Show comment
Hide comment
@kbrose

kbrose Oct 17, 2017

Contributor

... no discussion?

Contributor

kbrose commented Oct 17, 2017

... no discussion?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 17, 2017

Member

Sorry, this was more of a temporary fix. I'm open to discussion, I just wanted to revert it back to where we originally were so that it wouldn't seem like there was any time pressure.

Member

50Wliu commented Oct 17, 2017

Sorry, this was more of a temporary fix. I'm open to discussion, I just wanted to revert it back to where we originally were so that it wouldn't seem like there was any time pressure.

@kbrose

This comment has been minimized.

Show comment
Hide comment
@kbrose

kbrose Oct 17, 2017

Contributor

Ok, well as you can imagine I'm a -1 on this. Both versions give you incorrect indentation, the difference is the old-old version (that this reverts to) is incorrect on every end-of-block-like statements (i.e. return), whereas the old (before this revert) version has incorrect indentation in only some instances of those statements.

Contributor

kbrose commented Oct 17, 2017

Ok, well as you can imagine I'm a -1 on this. Both versions give you incorrect indentation, the difference is the old-old version (that this reverts to) is incorrect on every end-of-block-like statements (i.e. return), whereas the old (before this revert) version has incorrect indentation in only some instances of those statements.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 17, 2017

Member

Yeah. However, and maybe this is personal taste, I feel that having the indentation consistently not change is somewhat more expected than having the indentation unexpectedly change. For the former I can get used to shift-tabbing after every end statement, whereas for the latter there's more muscle memory involved, as well as some cursor movement (as shift-tab will work anywhere, but tabbing will only work at the beginning of the line).

Member

50Wliu commented Oct 17, 2017

Yeah. However, and maybe this is personal taste, I feel that having the indentation consistently not change is somewhat more expected than having the indentation unexpectedly change. For the former I can get used to shift-tabbing after every end statement, whereas for the latter there's more muscle memory involved, as well as some cursor movement (as shift-tab will work anywhere, but tabbing will only work at the beginning of the line).

@kbrose

This comment has been minimized.

Show comment
Hide comment
@kbrose

kbrose Oct 17, 2017

Contributor

No mouse touching needed - if you have at least one character highlighted tab will work as expected, so I trained a shift left-arrow tab (highlight one character to the left of the cursor and indent) muscle memory for myself.

Contributor

kbrose commented Oct 17, 2017

No mouse touching needed - if you have at least one character highlighted tab will work as expected, so I trained a shift left-arrow tab (highlight one character to the left of the cursor and indent) muscle memory for myself.

@kbrose

This comment has been minimized.

Show comment
Hide comment
@kbrose

kbrose Oct 18, 2017

Contributor

Did the tests run on this PR? I ran tests locally against master branch and had some failures. I had to update the spec file as seen in 7f8ea3b

Contributor

kbrose commented Oct 18, 2017

Did the tests run on this PR? I ran tests locally against master branch and had some failures. I had to update the spec file as seen in 7f8ea3b

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 18, 2017

Member

See my review comment in #214 - I'm not sure why but atom.workspace.open doesn't always work anymore on CI. Removing the filename fixes the issue (see atom/language-go@ca11612). I need to file an issue for that.

Member

50Wliu commented Oct 18, 2017

See my review comment in #214 - I'm not sure why but atom.workspace.open doesn't always work anymore on CI. Removing the filename fixes the issue (see atom/language-go@ca11612). I need to file an issue for that.

DSpeckhals added a commit to DSpeckhals/python-indent that referenced this pull request Nov 9, 2017

Handle missing "decreaseNextIndent" pattern
This pattern was recently removed from language-python.
See atom/language-python#212

This commit handles that scenario, as well as fixes tests surrounding
it. Other tests were adjusted to handle Atom not auto-inserting the
indentation for several scenarios where we are currently using the
`makeNewline` function. The tabs to indent (which are always a multiple
of the editor's tab length) can be passed. This only happens on Atom >
1.23. There is probably a better solution for this in the long term.

DSpeckhals added a commit to DSpeckhals/python-indent that referenced this pull request Nov 9, 2017

Handle missing "decreaseNextIndent" pattern
This pattern was recently removed from language-python.
See atom/language-python#212

This commit handles that scenario, as well as fixes tests surrounding
it. Other tests were adjusted to handle Atom not auto-inserting the
indentation for several scenarios where we are currently using the
`makeNewline` function. The tabs to indent (which are always a multiple
of the editor's tab length) can be passed. This only happens on Atom >
1.23. There is probably a better solution for this in the long term.

DSpeckhals added a commit to DSpeckhals/python-indent that referenced this pull request Nov 9, 2017

Handle missing "decreaseNextIndent" pattern (#55)
This pattern was recently removed from language-python.
See atom/language-python#212

This commit handles that scenario, as well as fixes tests surrounding
it. Other tests were adjusted to handle Atom not auto-inserting the
indentation for several scenarios where we are currently using the
`makeNewline` function. The tabs to indent (which are always a multiple
of the editor's tab length) can be passed. This only happens on Atom >
1.23. There is probably a better solution for this in the long term.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment