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

Better visualize differences in multiline strings #47

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

MaksimMisin
Copy link
Contributor

@MaksimMisin MaksimMisin commented Apr 5, 2024

I had the same issue as #14 where my multiline string difference was not very readable:
image

I added a command line option --icdiff-multiline-string. Running the same test with pytest --icdiff-multiline-string -vv now returns a significantly more readable diff:
image

@hjwp
Copy link
Owner

hjwp commented Apr 5, 2024

thanks very much for the suggestion, and the PR! I think I'd defintely want something to improve string diffs.

It occurs to me - wouldnt' we maybe want to do this kind of non-pprint diff on any string? like, all the time, and then we wouldn't need a special CLI option?

i'd be keen to merge this without the CLI option, and just with an if that checks if both left and right are str.

also we'd need a test.

@MaksimMisin
Copy link
Contributor Author

Thanks for the comments, I only added this option because I was afraid the pprint-ed strings were useful in certain situations.

LMK if you want more tests added.

Comment on lines +242 to +244
assert "a" + " " * 36 in output
assert "b" + " " * 36 in output
assert "c" + " " * 36 in output
Copy link
Owner

Choose a reason for hiding this comment

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

i don't quite understand what these asserts are doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's to make sure that there're actually empty spaces instead of new lines after last characters on the line
image

before it looked like this:
image
so this test would fail

36 should be slightly above the actual size of these columns, with some margins in case icdiff implementation changes a bit.

@MaksimMisin
Copy link
Contributor Author

Do you think you can merge the PR or is there anything missing?

@hjwp hjwp merged commit 745b476 into hjwp:main Apr 15, 2024
@hjwp
Copy link
Owner

hjwp commented Apr 15, 2024

merged with thanks! I might try and find the time to fiddle with that test assertion to make it more readable, but otherwise all good for now! will throw up a new release when i get a moment...

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 this pull request may close these issues.

2 participants