add context lines to matches #195
Conversation
|
Since the beta of 1.15 was just released it would be great if this extended API could be merged to become available in the next beta (1.16). |
src/text-buffer.coffee
Outdated
| # * `lineCountBefore` {Number} default `0`; The number of lines before the | ||
| # matched line to include in the results object. | ||
| # * `lineCountAfter` {Number} default `0`; The number of lines after the | ||
| # matched line to include in the results object. |
There was a problem hiding this comment.
After talking with @nathansobo about this new API, we decided that the following names would tell the story better:
lineCountBefore->leadingContextLineCountlineCountAfter->trailingContextLineCountlinesBefore->leadingContextLineslinesAfter->trailingContextLines
Could you make those changes to the field names? I'll update scandal to match those names.
src/text-buffer.coffee
Outdated
| break if row >= @getLineCount() | ||
| argument.linesAfter.push(@lineForRow(row)) | ||
|
|
||
| callback(argument) |
There was a problem hiding this comment.
Since scanInRange is called a lot, I'd like to avoid creating this wrapper closure. Could you move this functionality to the SingleLineSearchCallbackArgument and MultiLineSearchCallbackArgument classes?
I think that iterator.iterate could take leadingContextLineCount and trailingContextLineCount as two new parameters, and could pass them to the callback arguments' constructors.
src/text-buffer.coffee
Outdated
| for i in [0...(options.lineCountBefore or 0)] | ||
| row = argument.range.start.row - i - 1 | ||
| break if row < 0 | ||
| argument.linesBefore.unshift(@lineForRow(row)) |
There was a problem hiding this comment.
Since unshift has to move every element in the array, I think it'd be slightly more direct and fast to iterate through the buffer's lines in the forwards direction. Maybe something like:
row = Math.max(0, range.start.row - lineCountBefore)
while row < range.start.row
argument.linesBefore.push(@lineForRow(row))
|
I updated the code based on your feedback. I moved the logic to populate the context lines here so that both One thing really confusing: Travis shows up "green" even if some tests are failing. That is probably something which should be addressed outside of this PR. |
|
All tests seem to be passing though? |
|
Yeah, now they are. Before I had some commits which failed some tests but Travis was still green. I made an example PR to demonstrate the problem. Please see #222. |
src/match-iterator.coffee
Outdated
| constructor: (@buffer, @regex, range, @options={}, @chunkSize) -> | ||
| if _.isNumber(@options) | ||
| @chunkSize = @options | ||
| @options = {} |
There was a problem hiding this comment.
Why is a conditional needed here? Don't we always pass the chunkSize argument? I'd think that we could make options the last argument.
There was a problem hiding this comment.
The three other classes don't have a chunkSize argument. I tried to keep the order of the common arguments the same (therefore options after range). The conditional is there to not break compatibility if any other code is calling this API.
There was a problem hiding this comment.
I see what you're saying; thanks for being careful about compatibility. In this case though, these classes are private and their constructors don't have to have the same signature.
There was a problem hiding this comment.
Ok, no problem. I updated the patch accordingly.
|
Thanks @dirk-thomas; Great work! |
|
⚡️ ⚡️ ⚡️ |
This is in support of atom/find-and-replace#847.
The PR provide similar functionality as atom/scandal#41. It adds an optional
optionsargument to thescanfunctions. The options can containlineCountBeforeandlineCountAfter. The return results now contain additionallylinesBeforeandlinesAfter.The previous signatures should continue to work without any need to change existing code.