Add option to specify context size #14
Conversation
There was a problem hiding this comment.
Thanks a lot for contributing this!
When I looked at the code last time, I had the same intuition, but didn't get around to it.
Something that's gravely missing, but should be easy to add, is a test - there are already some that could be used as template.
Thanks again!
src/unified_diff.rs
Outdated
| @@ -116,11 +119,11 @@ where | |||
| type Out = W; | |||
|
|
|||
| fn process_change(&mut self, before: Range<u32>, after: Range<u32>) { | |||
| if before.start - self.pos > 6 { | |||
| if before.start - self.pos > self.ctx_size { | |||
There was a problem hiding this comment.
Isn't this 2 * self.ctx_size?
There was a problem hiding this comment.
I did not understand why it would have to be 2 * self.ctx_size. For me, we want to move self.pos to before.start - self.ctx_size if it's not already inside that range. But maybe I'm missing something, that will be clearer once I write some tests (or @pascalkuthe chimes in).
There was a problem hiding this comment.
I'd start at fixing CI before pinging people.
There was a problem hiding this comment.
OK so indeed 2 * self.ctx_size works well when we are between 2 hunks. But for the first hunk it can be incorrect. For example, say you have ctx_size=5. First hunk starts at line 9, self.pos is currently at 0. Since 9 - 0 < 2 * 5, you wont move self.pos and will print 8 lines of context before the first hunk.
There was a problem hiding this comment.
Apologies for getting back to you so late - I wasn't aware that all of the issues have been addressed.
There is one more shortcoming regarding the documentation which I think could be fixed in a way that also allows extension in the future. It's something I'd implement myself, but then I can't merge myself anymore.
Thanks again for your help!
Byron
force-pushed
the
context_size_option
branch
from
c3b8fc3 to
ba2fa5e
Compare
There was a problem hiding this comment.
Thanks a lot!
I have rewritten the last commit to keep the option open to merge it myself I suppose, but I think this breaking change is now made so that other types of context could be added in the future without additional breaking changes.
The diff is best viewed by hiding whitespace, as I moved code a round a bit.
I will leave the merging to @pascalkuthe though just to be sure this works for him.
Byron
force-pushed
the
context_size_option
branch
from
ba2fa5e to
0832791
Compare
That way it's possible to update it in future, for instance to add assymetric context options. Co-authored-by: Sebastian Thiel <[email protected]> 1733756156 +0100
Byron
force-pushed
the
context_size_option
branch
from
0832791 to
8517f10
Compare
This is the initial state as pulled from pascalkuthe/imara-diff#14 at state pascalkuthe/imara-diff@8517f10 The idea is that once it stabilized for us we can hopefully contribute it back. We'd also want to eventually use the released version of `imara-diff`.
This is the initial state as pulled from pascalkuthe/imara-diff#14 at state pascalkuthe/imara-diff@8517f10 The idea is that once it stabilized for us we can hopefully contribute it back. We'd also want to eventually use the released version of `imara-diff`.
This is the initial state as pulled from pascalkuthe/imara-diff#14 at state pascalkuthe/imara-diff@8517f10 The idea is that once it stabilized for us we can hopefully contribute it back. We'd also want to eventually use the released version of `imara-diff`.
This is the initial state as pulled from pascalkuthe/imara-diff#14 at state pascalkuthe/imara-diff@8517f10 The idea is that once it stabilized for us we can hopefully contribute it back. We'd also want to eventually use the released version of `imara-diff`.
similar to
diff -U NUM