Skip to content
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 is reversed #7853

Closed
bluthej opened this issue Oct 8, 2023 · 3 comments · Fixed by #7855
Closed

Diff is reversed #7853

bluthej opened this issue Oct 8, 2023 · 3 comments · Fixed by #7855

Comments

@bluthej
Copy link
Contributor

bluthej commented Oct 8, 2023

Issue

The diff output is reversed since #7813.

For example, if I give ruff the following input file:

from .logging import config_logging
from .settings import ENV
from .settings import *

then I get the following correct output with v0.0.292:

--- test.py
+++ test.py
@@ -1,3 +1,3 @@
 from .logging import config_logging
+from .settings import *
 from .settings import ENV
-from .settings import *

Would fix 1 error.

But with the main branch, I get:

--- test.py
+++ test.py
@@ -1,3 +1,3 @@
 from .logging import config_logging
+from .settings import ENV
 from .settings import *
-from .settings import ENV

Would fix 1 error.

Fix

I traced it back to this line, where the old and new source files are reversed. It should read:

let text_diff = TextDiff::from_lines(src, dst);

If that's ok, I can submit a quick PR to fix it. There are 2 tests whose snapshot needs to be updated as a consequence, but apart from that the only change needed is the one I mentioned above.

@zanieb
Copy link
Member

zanieb commented Oct 8, 2023

Thanks! I wonder how that pull request didn't change any snapshots

@charliermarsh
Copy link
Member

Oops, PR welcome, thank you.

@bluthej
Copy link
Contributor Author

bluthej commented Oct 9, 2023

@zanieb it seems like there are only two integration tests that test the output of the CLI diff option (diff_shows_safe_fixes_by_default and diff_shows_unsafe_fixes_with_opt_in), and they were added in #7769, which was merged 2 days after #7813 ^^

dhruvmanila pushed a commit that referenced this issue Oct 9, 2023
## Summary

Fixes #7853.

The old and new source files were reversed in the call to
`TextDiff::from_lines`, so the diff output of the CLI was also reversed.

## Test Plan

Two snapshots were updated in the process, so any reversal should be
caught :)
konstin pushed a commit that referenced this issue Oct 11, 2023
## Summary

Fixes #7853.

The old and new source files were reversed in the call to
`TextDiff::from_lines`, so the diff output of the CLI was also reversed.

## Test Plan

Two snapshots were updated in the process, so any reversal should be
caught :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants