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

feature: relative, plus/minus ranges. closes #2384 #3071

Merged
merged 11 commits into from Oct 11, 2018

Conversation

captaincaius
Copy link
Contributor

@captaincaius captaincaius commented Sep 23, 2018

What this PR does / why we need it:
This PR fixes the current line ranges functionality and also adds the ability to do relative line ranges like vim (e.g. :.,.+2s/this/that/). A few bugs were fixed along the way too (e.g. specifying a single-line only, etc).

Which issue(s) this PR fixes
#2384

Special notes for your reviewer:

First of all, you'll notice I tried to make it behave like vim even when in undocumented cases. If anything seems a little weird, there'll be a comment with "undocumented:" nearby so you can try it for yourself on vim ;)

Also, a few things worth noting that i came across while working on this:

  • I decided to defer the shorthand feature in vim, because it's unrelated - i.e. 4:s/this/that becomes :.,.+3s/this/that done in this PR - it turned out to be a 4-liner ;)
  • I think TokenType.LineNumber probably needs a different name, like LineNumberOrOffset ;) - or we can do some surgery on the lexer, but I don't think you folks would think that's worthwhile. I updated the lexer - I decided it's clearer to understand this way, even though it's a little more code to read. If anyone disagrees, I'll be happy to revert.
  • Another separate feature that needs a followup issue is related to cursor position after ranged commands - I noticed we're not mimicking vim on this (neither on relative nor absolute ranges).
  • I haven't looked around too much, but is it intentional that interpreting the range is delegated to the commands themselves? For example, the single-line bug I found was fixed only in substitute, but it might also exist in other commands.
  • Speaking of which, I left a bit of residue for "Percent" in lineRefToPosition - again, since it seems that responsibility is left to the commands, I don't think it was being used. But I didn't want to touch it without discussing it first.

@captaincaius
Copy link
Contributor Author

captaincaius commented Oct 2, 2018

Since nobody's had time to review this yet, I decided to take care of the second point above so it's a little cleaner within this PR :). I took the second approach (improving the lexer so LineNumber and Offset are semantically distinct). I'll update it to master and then push it.

captaincaius and others added 4 commits October 2, 2018 13:44
It seems more in-line with the original direction the code was going
It's a little more code, but I think easier to understand for a
first-time reader this way.
return lexDigits(TokenType.LineNumber);
} else {
// otherwise, use previous token to determine which flavor of digit lexer should be used
let previousTokenType = tokens[tokens.length - 1].type;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

let currentColumn = 0; // only mark does this differently
let currentOperation: LineRefOperation = undefined;

var firstToken = toks[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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.

None yet

2 participants