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
diff-lcs 1.4.4 - Fix broken formatting #66
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
halostatue
force-pushed
the
misplaced-chunk-issue-65
branch
from
July 1, 2020 00:30
d3a9ad5
to
dbb36e6
Compare
@JonRowe, this is going to fix an issue that RSpec may not have (because RSpec diffs tend to be small), but I wanted to get some input (still looking at test failures, so this is definitely still draft). |
This was
linked to
issues
Jul 1, 2020
Closed
# This is the 1st commit message: Fix improperly placed chunks Resolve #65 - Also add even more tests for checking `ldiff` results against `diff` results. - Fix issues with diff/ldiff output highlighted by the above tests. - Add a parameter to indicate that the hunk being processed is the _last_ hunk; this results in correct counting of the hunk size. - The misplaced chunks were happening because of an improper `.abs` on `#diff_size`, when the `.abs` needed to be on the finding of the maximum diff size. # This is the commit message #2: Ooops. Debugger # This is the commit message #3: Restore missing test - Fix some more format issues raised by the missing test. - Start fixing Rubocop formatting. # This is the commit message #4: Last RuboCop fixes # This is the commit message #5: Finalize diff-lcs 1.4 # This is the commit message #6: Fix #44 The problem here was the precedence of `or` vs `||`. Switching to `||` resulted in the expected behaviour. # This is the commit message #7: Resolve #43 # This is the commit message #8: Typo # This is the commit message #9: Resolve #35 with a comment
- Resolve #65: Two different issues were reported: - Line numbers changed between 1.3 and 1.4. This change is intentional, but in comparing the output against `diff` for the new examples provided it became clear that for unified and context diffs, the last hunk was reporting incorrect hunk lengths (that is, if it was `12,4 11,8`, it should have been `12,3 11,7`. This has been resolved by providing `Diff::LCS::Hunk#diff` an additional parameter, `last` which defaults to `false`. - Net new lines were added in the middle of a unified diff hunk, which was incorrect. That is, we were getting: ```diff @@ -10,6 +11,10 @@ recipe[j::default] recipe[k::default] recipe[l::default] +recipe[o::new] +recipe[p::new] +recipe[q::new] +recipe[r::new] recipe[m::default] recipe[n::default] ``` instead of: ```diff @@ -12,3 +11,7 @@ recipe[l::default] recipe[m::default] recipe[n::default] +recipe[o::new] +recipe[p::new] +recipe[q::new] +recipe[r::new] ``` This has been resolved. The error was that `Diff::LCS::Block#diff_size` was applying `.abs` to its output, which is incorrect. We need to know the direction of the size for placing changes, but when determining the maximum block size for use in `Diff::LCS::Hunk#diff` calculations, we need to know the `.abs` size. - New tests were added to prevent these changes from regressing in the future, both as issue tests and as additional `ldiff` tests. These tests highlighted more issues with diff-lcs output as compared to `diff`, specifically the handling and reporting of missing newlines at the end of files. All of the issues highlighted were resolved and the structure of `ldiff` tests was changed so that it is easier to add new comparison files at any time. - Resolve #35: Indicate that when comparing against custom objects, `#eql?` must be implemented such that objects that _resolve_ to the same meaning are _treated_ as the same meaning. This is important because the basic LCS algorithm uses a hash for position matching. - Resolve #43: Provide a more meaningful error from `Diff::LCS::Hunk.new` if the `piece` provided does not create useful `Diff::LCS::Block`. It's extremely unlikely, this error will be more useful than `NoMethodError` being thrown. - Resolve #44: `ldiff` binary detection failed to work correctly. It had been `!old_text or !new_text`, but the precedence of `or` broke that. It is now `!old_text || !new_text`. Also: - Ran Rubocop again. Updated configuration definitions, fixed some code formatting.
halostatue
force-pushed
the
misplaced-chunk-issue-65
branch
from
July 1, 2020 16:40
aa3a9a4
to
b75e0bd
Compare
halostatue
changed the title
Misplaced Diff::LCS::Hunk when unified
diff-lcs 1.4.4 - Fix broken formatting
Jul 1, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolve Diff::LCS::Hunk#diff(:unified) output changed between 1.3 and 1.4.x, part 2 #65: Two different issues were reported:
Line numbers changed between 1.3 and 1.4. This change is intentional, but in comparing the output against
diff
for the new examples provided it became clear that for unified and context diffs, the last hunk was reporting incorrect hunk lengths (that is, if it was12,4 11,8
, it should have been12,3 11,7
. This has been resolved by providingDiff::LCS::Hunk#diff
an additional parameter,last
which defaults tofalse
.Net new lines were added in the middle of a unified diff hunk, which was incorrect. That is, we were getting:
instead of:
This has been resolved. The error was that
Diff::LCS::Block#diff_size
was applying.abs
to its output, which is incorrect. We need to know the direction of the size for placing changes, but when determining the maximum block size for use inDiff::LCS::Hunk#diff
calculations, we need to know the.abs
size.New tests were added to prevent these changes from regressing in the future, both as issue tests and as additional
ldiff
tests. These tests highlighted more issues with diff-lcs output as compared todiff
, specifically the handling and reporting of missing newlines at the end of files. All of the issues highlighted were resolved and the structure ofldiff
tests was changed so that it is easier to add new comparison files at any time.Resolve Incorrect results when Enumerable contains generic objects. String collections works. #35: Indicate that when comparing against custom objects,
#eql?
must be implemented such that objects that resolve to the same meaning are treated as the same meaning. This is important because the basic LCS algorithm uses a hash for position matching.Resolve Make Diff::LCS::Hunk Robust #43: Provide a more meaningful error from
Diff::LCS::Hunk.new
if thepiece
provided does not create usefulDiff::LCS::Block
. It's extremely unlikely, this error will be more useful thanNoMethodError
being thrown.Resolve Diff::LCS::Ldiff @binary #44:
ldiff
binary detection failed to work correctly. It had been!old_text or !new_text
, but the precedence ofor
broke that. It is now!old_text || !new_text
.Also: