added lines and deleted lines in content changes #1790
Conversation
|
This looks good to me, thanks @Stijn-Rutten. @bording any thoughts? |
bording
left a comment
There was a problem hiding this comment.
Overall looks good, but I have a few tweaks I'd like to see, and a couple of questions.
| /// </summary> | ||
| public virtual List<Line> DeletedLines { get; internal set; } | ||
|
|
||
|
|
There was a problem hiding this comment.
Still need to clean up this extra line break.
LibGit2Sharp/Patch.cs
Outdated
| PatchEntryChanges currentChange = this[filePath]; | ||
| string prefix = string.Empty; | ||
|
|
||
| string decodedContent = LaxUtf8Marshaler.FromNative(line.content, (int)line.contentLen); |
There was a problem hiding this comment.
Why do we need this when this appears to be the same as the patchPart declaration on line 59?
There was a problem hiding this comment.
This should able to be removed. You can just use the existing patchPart instead.
LibGit2Sharp/Patch.cs
Outdated
|
|
||
| string decodedContent = LaxUtf8Marshaler.FromNative(line.content, (int)line.contentLen); | ||
|
|
||
| currentChange.AddedLines = new List<Line>(); |
There was a problem hiding this comment.
Why are these needed? Seems like this should be already handled in the ContentChanges constructor.
There was a problem hiding this comment.
I have added this in case the contructor that calls LineCallback on ContentChanges is not the constructor that is utilized by Patch. This way these values are always set. After fixing the declaration of the lists and adding tests. This does work. I would like to get your input on whether or not you think this is a viable option.
…because linecallback isn't called in this case
Co-authored-by: Brandon Ording <bording@gmail.com>
Co-authored-by: Brandon Ording <bording@gmail.com>
bording
left a comment
There was a problem hiding this comment.
There are a lot of unrelated formatting changes in the test files. It's a good practice to avoid those kind of unrelated changes in a PR, but in this case let's go ahead and keep them. I've made some specific comments about a couple changes I'd like to see undone, though.
| /// </summary> | ||
| public virtual List<Line> DeletedLines { get; internal set; } | ||
|
|
||
|
|
There was a problem hiding this comment.
Still need to clean up this extra line break.
LibGit2Sharp/Patch.cs
Outdated
| PatchEntryChanges currentChange = this[filePath]; | ||
| string prefix = string.Empty; | ||
|
|
||
| string decodedContent = LaxUtf8Marshaler.FromNative(line.content, (int)line.contentLen); |
There was a problem hiding this comment.
This should able to be removed. You can just use the existing patchPart instead.
…ttps://github.com/Stijn-Rutten/libgit2sharp into 1789_addedLines_and_deletedLines_in_ContentChanges
|
Checks seem to be stuck, can someone force a restart? |
|
@bording @ethomson @Stijn-Rutten How's this PR coming along? Looks like a lot was fixed since the last review |
|
@Mobrockers I processed @bording 's feedback and am waiting for a new review. |
|
@ethomson @bording @Mobrockers Could you take a look at this. We have been waiting for a while and could really use this feature. |
|
@bording 🎉🎉🎉🎉🎉🎉🎉🎉 |
|
FYI these changes are part of 0.27.0-preview-0096, which was just published. |
This PR is related to issue #1789