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

Add support for indent objects #1550

Merged
merged 9 commits into from
Apr 27, 2017
Merged

Add support for indent objects #1550

merged 9 commits into from
Apr 27, 2017

Conversation

mikew
Copy link
Contributor

@mikew mikew commented Apr 22, 2017

As defined at michaeljsmith/vim-indent-object.

Supports ai, ii, and aI with c, d, and v operators.

Refs #1096.

@mikew
Copy link
Contributor Author

mikew commented Apr 22, 2017

Actually, looks like I broke some logic when I refractored before final submission. The tests aren't specific enough

@mikew
Copy link
Contributor Author

mikew commented Apr 22, 2017

Hmmm, that failing test wasn't modified by this PR and passed the first time. I'm guessing a maintainer can retry the build.

I do have some questions though:

  1. I made a class IndentObjectMatch extends BaseMovement in actions.ts but I'm not sure the use of it. I based it off of MoveTagMatch with some logic removed.
  2. TextEditor.getLineMaxColumn errors when dealing with line 0 when it seems like there's nothing stopping it from working. I've worked around this by using TextEditor.readLineAt(endLineNumber).length
  3. I'm not sure how to implement the d operator properly. As currently implemented, dii will leave you with a blank line, when really that line should be gone too. I couldn't figure out how to get rid of it without also removing the first character of the next line.

@jpoon
Copy link
Member

jpoon commented Apr 22, 2017

Restarted. Not the best person to review though.

@mikew
Copy link
Contributor Author

mikew commented Apr 23, 2017

Appreciated. Though I'm only just now noticing the existing TextObjectMovement class. I can easily refactor all logic into that style.

Also changes IndentObjectMatch to subclass TextObjectMovement
@johnfn
Copy link
Member

johnfn commented Apr 27, 2017

Wow, impressive work! Thanks for the contribution.

Would you mind adding a little blurb in the README to explain what's going on here/how to use these guys?

And yeah if you want to refactor to the TextObjectMovement class too, that would be awesome. It's not necessary for the merge though, so just let me know.

@mikew
Copy link
Contributor Author

mikew commented Apr 27, 2017

Already done days ago, just never pushed. Working on the README now but it seems it's changed a lot in master.

@johnfn
Copy link
Member

johnfn commented Apr 27, 2017

Yeah, we had a PR a few days ago that totally rewrote it. :) Should be stable-ish now.

…-object

* 'master' of https://github.com/VSCodeVim/Vim: (27 commits)
  bump package version
  fix gq reflow cursor position
  bump package version
  Fixes #1573
  Fix logo src so logo displays inside VSCode (#1572)
  fixes #1449 (#1571)
  fixes #1570
  fixes #1235
  Fix position diff in gq reflow
  fixes #1252
  fixes #1486
  Update README.md
  Update ROADMAP.md
  fixes #1357
  fixes #1563 fixes #1562
  bump package version
  update clipboardy library with windows utf-8 fix
  Squashed lines together
  Escapes newlines in register display
  README enhancements (#1547)
  ...
@mikew
Copy link
Contributor Author

mikew commented Apr 27, 2017

I merged master and updated the README. I decided against examples since it seemed to add a lot and instead went with comparing it to cit/ci{/ci[/ciB

@johnfn
Copy link
Member

johnfn commented Apr 27, 2017

Looks good to me. It's kind of funny since I added the custom af command to VSCodeVim to do (nearly) the exact same thing. Eh, what are you going to do?

Thanks again for the awesome work. :)

@johnfn johnfn merged commit c57d370 into VSCodeVim:master Apr 27, 2017
@mikew
Copy link
Contributor Author

mikew commented Apr 27, 2017

Glad to help. And I was just thinking it would be cool if the count prefix could mean "look n levels up". As it stands, canBePrefixedWithCount should be false. Missed that one.

@mikew mikew deleted the indent-object branch April 27, 2017 07:20
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants