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.

Sign up for GitHub

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

Jump to bottom

Multiline diff formatting improvements #137

Closed
wants to merge 11 commits into from
Closed

Multiline diff formatting improvements #137

wants to merge 11 commits into from

Conversation

dbader
Copy link
Contributor

@dbader dbader commented Dec 12, 2020

I've been working on some improvements to make the diff viewer experience better. This changeset fixes whitespace rendering if there are leading whitespace characters on a line and there are also visual changes to make it easier to see which lines have changed in a multiline diff.

Changes:

  • mark modified lines
  • fix leading whitespace

Before:

Screen Shot 2020-12-11 at 5 43 33 PM

After:

multiline-diff-improvements

@dbader
Multiline diff improvements
f76c759 
- mark modified lines
- fix <pre> whitespace
dbader
dbader commented Dec 12, 2020
View reviewed changes
reversion_compare/helpers.py Outdated Show resolved Hide resolved
@dbader
Copy link
Contributor Author

dbader commented Dec 12, 2020

@jedie Thanks for this super handy module :) Opening this as a preliminary PR tp get some input on these changes. I'm happy to round it out with tests etc whatever is needed to move forward. Curious to hear your thoughts when you've had a chance to take a look.

@jedie
Copy link
Owner

jedie commented Dec 12, 2020

Your example output looks great👌
Thanks for contribution.

@dbader
Copy link
Contributor Author

dbader commented Dec 12, 2020

I've removed the \r\n line endings assumption. I should probably also add a test for diff_match_patch_pretty_html() to test_helpers.py.

@dbader
Copy link
Contributor Author

dbader commented Dec 12, 2020

I need to revisit some of the test case changes to make sure the added <span> elements make sense. Running out of time today but I'll do it over the next few days.

@dbader
Copy link
Contributor Author

dbader commented Dec 13, 2020

Quick update: There are still some issues with newline insertions/deletions that I need to investigate, so this is still a work-in-progress.

@dbader
Copy link
Contributor Author

dbader commented Dec 13, 2020

Current status:

  • diff-line spans generated correctly in the HTML even if insert/delete diff ops cross a line boundary
  • updated color scheme for change indicators based on GitHub PR diff style (higher contrast and better visibility -- this is of course quite subjective, happy to adjust further)
  • indicate inserted/deleted line feeds with ⏎ character

Screenshot of the diff visualization generated by f07ba91:

Screen Shot 2020-12-13 at 1 00 15 PM

@dbader
Copy link
Contributor Author

dbader commented Dec 14, 2020

Okay, I think this is good to go :) Tests are passing locally for me but seem to be affected by the same issue on TravisCI that's also breaking master.

dbader
dbader commented Dec 14, 2020
View reviewed changes
reversion_compare_tests/models.py
@@ -104,7 +105,7 @@ class VariantModel(models.Model):

TEST_CHOICES = (("a", "alpha"), ("b", "bravo"))
boolean = models.BooleanField(default=True)
null_boolean = models.NullBooleanField()
null_boolean = models.BooleanField(null=True) if DJANGO_VERSION[0] >= 3 else models.NullBooleanField()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jedie FYI this is unrelated to the diff changes, but it fixes a deprecation warning that was causing a test failure.

@dbader
Copy link
Contributor Author

dbader commented Dec 14, 2020

Tiny CSS tweak to ensure diff lines are always full-width to make changes easier to see while scrolling through a large diff:

Screen Shot 2020-12-14 at 9 54 51 AM

@jedie
Copy link
Owner

jedie commented Dec 23, 2020

can you rebase?

@dbader
Copy link
Contributor Author

dbader commented Jan 31, 2021 •
edited
Loading

@jedie This should be ready to go -- just wanted to check in to see if you needed anything else from me to move forward? (no rush obviously, I know you're busy)

jedie added a commit that referenced this pull request Feb 4, 2021
@jedie
Multiline diff formatting improvements #137
c086662 
Squashed commit of the following:

commit d48d60cb40090c7fb2c8b6a97a134100e12f07a0
Author: Dan Bader <[email protected]>
Date:   Mon Dec 14 09:55:24 2020 -0800

    Make diff lines full-width

commit b17c63359dcbd9ac0c704a18c11875202d7d7fbe
Author: Dan Bader <[email protected]>
Date:   Sun Dec 13 18:51:59 2020 -0800

    Cleanup; add docstrings

commit ad0383cdc4e1822d014483839c7d69e9747c0548
Author: Dan Bader <[email protected]>
Date:   Sun Dec 13 12:59:45 2020 -0800

    Update diff color scheme

commit 9f4ac80d2991dbb8c87614ad1d4388711cbc0e4e
Author: Dan Bader <[email protected]>
Date:   Sun Dec 13 12:59:28 2020 -0800

    Fix diff_match_patch_pretty_html HTML spans

commit 1ed6665be6f4359b8a89e153383fc9f6b89cbe4b
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 16:39:05 2020 -0800

    Fix line end handling, update tests

commit 111e59b08b55a6f05d431dfc26df5a805cfe7b31
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 16:10:35 2020 -0800

    Update test_variant_model.py

commit dd7fc4d1499395bcb6e76233e5529958f236665a
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 16:10:18 2020 -0800

    Fix NullBooleanField deprecation warning

commit 13a38f6728365b7fad52df4cba9c6af004493dc0
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 11:33:46 2020 -0800

    Update test_helpers.py

commit ca21fa765580f066542b0da343638803e3d85d7b
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 10:24:00 2020 -0800

    Diff viewer improvements: Don't assume \r\n line endings

commit 173429c8321e1530eb31988fde6880c245cb25f3
Author: Dan Bader <[email protected]>
Date:   Fri Dec 11 18:03:33 2020 -0800

    Multiline diff improvements

    - mark modified lines
    - fix <pre> whitespace
jedie added a commit that referenced this pull request Feb 4, 2021
@jedie
Multiline diff formatting improvements #137
743a135 
Some small code changes and the squashed commits of the following:

commit d48d60cb40090c7fb2c8b6a97a134100e12f07a0
Author: Dan Bader <[email protected]>
Date:   Mon Dec 14 09:55:24 2020 -0800

    Make diff lines full-width

commit b17c63359dcbd9ac0c704a18c11875202d7d7fbe
Author: Dan Bader <[email protected]>
Date:   Sun Dec 13 18:51:59 2020 -0800

    Cleanup; add docstrings

commit ad0383cdc4e1822d014483839c7d69e9747c0548
Author: Dan Bader <[email protected]>
Date:   Sun Dec 13 12:59:45 2020 -0800

    Update diff color scheme

commit 9f4ac80d2991dbb8c87614ad1d4388711cbc0e4e
Author: Dan Bader <[email protected]>
Date:   Sun Dec 13 12:59:28 2020 -0800

    Fix diff_match_patch_pretty_html HTML spans

commit 1ed6665be6f4359b8a89e153383fc9f6b89cbe4b
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 16:39:05 2020 -0800

    Fix line end handling, update tests

commit 111e59b08b55a6f05d431dfc26df5a805cfe7b31
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 16:10:35 2020 -0800

    Update test_variant_model.py

commit dd7fc4d1499395bcb6e76233e5529958f236665a
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 16:10:18 2020 -0800

    Fix NullBooleanField deprecation warning

commit 13a38f6728365b7fad52df4cba9c6af004493dc0
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 11:33:46 2020 -0800

    Update test_helpers.py

commit ca21fa765580f066542b0da343638803e3d85d7b
Author: Dan Bader <[email protected]>
Date:   Sat Dec 12 10:24:00 2020 -0800

    Diff viewer improvements: Don't assume \r\n line endings

commit 173429c8321e1530eb31988fde6880c245cb25f3
Author: Dan Bader <[email protected]>
Date:   Fri Dec 11 18:03:33 2020 -0800

    Multiline diff improvements

    - mark modified lines
    - fix <pre> whitespace
jedie added a commit that referenced this pull request Feb 4, 2021
@jedie
Merge pull request #144 from jedie/diff-improvments
a2d55bc 
Multiline diff formatting improvements #137
@jedie
Copy link
Owner

jedie commented Feb 4, 2021

I merged your code via #144 (I made some additional small changes)

@jedie jedie closed this Feb 4, 2021
@jedie
Copy link
Owner

jedie commented Feb 4, 2021

merged and release with v0.13.1
Thanks for contribution!

@jedie jedie mentioned this pull request Feb 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dbader @jedie