count as context when already reached limit#1642
count as context when already reached limit#1642zyctree wants to merge 1 commit intoBurntSushi:masterfrom
Conversation
zyctree
force-pushed
the
fix-m-A-together
branch
from
cc37b44 to
1d6049e
Compare
|
@BurntSushi can this be merged |
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
Thanks for taking a stab at this! I think I buy the fix, but would like to see this polished up a bit before merging.
| } else { | ||
| self.after_context_remaining = searcher.after_context() as u64; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you add a regression test at the bottom of this file so that we cover the JSON printer? It should be sufficient to mimic the existing tests where you only need to count the lines of output.
| self.after_context_remaining.saturating_sub(1); | ||
| } else { | ||
| self.after_context_remaining = searcher.after_context() as u64; | ||
| } |
There was a problem hiding this comment.
Could you also add a regression test at the bottom of this file? I know you added a regression integration test, but it would be good to cover it here too.
| ) -> Result<bool, io::Error> { | ||
| self.match_count += 1; | ||
| self.after_context_remaining = searcher.after_context() as u64; | ||
| if self.match_more_than_limit() { |
There was a problem hiding this comment.
Could you add a comment explaining this logic? (And then I guess copy the comment to the JSON printer as well.) It took me about 5 minutes of re-reading this code carefully to understand it.
| self.after_context_remaining == 0 | ||
| } | ||
|
|
||
| fn match_more_than_limit(&self) -> bool { |
There was a problem hiding this comment.
Could you add a small comment documenting this function? It would be useful to say, for example, that this always returns false if there is no limit. (And then copy the comment to the JSON printer.)
BurntSushi
added
the
rollup
2 participants
it will fix #1380, and it is a breaking change