Issue 6757 - Improved diff output for similar strings. #7099

piotrhm · 2020-04-19T15:47:01Z

This pull request implements suggested approach for issue #6757. Visualization:

changelog/6757.feature.rst

bluetech

I don't have an opinion on this feature (yet?), but left some minor comments/suggestions.

src/_pytest/assertion/util.py

changelog/6757.feature.rst

bluetech · 2020-04-19T20:27:25Z

Also, note that the "linting" CI check failed, due to formatting errors. You can fix it by running tox -e linting (or running black manually). You also check the logs and apply the necessary changes manually.

nicoddemus

Thanks a lot @piotrhm for working on this, we appreciate it.

Besides the comments left by @bluetech and me, could you please also add a test for this functionality? I like the cow example. 😁

changelog/6757.feature.rst

src/_pytest/assertion/util.py

src/_pytest/terminal.py

src/_pytest/assertion/util.py

nicoddemus · 2020-05-04T21:36:53Z

Thanks a lot @piotrhm, great work!

nicoddemus · 2020-06-09T17:34:33Z

@bluetech would you like to take a second look?

nicoddemus · 2020-06-10T20:18:42Z

Also, @piotrhm meanwhile could you rebase and solve the conflict there? Thanks!

piotrhm · 2020-06-11T11:40:40Z

Done. Thanks a lot @nicoddemus!

nicoddemus · 2020-06-13T14:48:35Z

@bluetech gentle ping for a final review. 👍

bluetech

I left some code comments. Some high level comments as well:

I'm confused about the desired behavior. The PR title says "Improve diff output for similar strings"; the changelog says "if the texts differ significantly (more than 30% of lines and more than 4 lines)", the actual code triggers the new behavior on s.ratio() >= 0.30 and max(nlines_left, nlines_right) >= 5 (that is 30% similarity). Logically the changelog sounds right (I think), but the code does the opposite.
I believe that using screen_width as done here is problematic because the explanation can be indented, e.g. in the new "drill down" mode and possibility other cases, in which case the =s will overflow and look funny. Somehow the indentation needs to be account for (though I don't think the code is built for that), or a solution that doesn't rely on screen width should be used.

bluetech · 2020-06-15T17:05:06Z

src/_pytest/assertion/util.py

@@ -131,6 +132,8 @@ def isiterable(obj: Any) -> bool:
 def assertrepr_compare(config, op: str, left: Any, right: Any) -> Optional[List[str]]:
    """Return specialised explanations for some operators/operands"""
    verbose = config.getoption("verbose")
+    screen_width = config.get_terminal_reporter().screen_width


Instead of adding get_terminal_reporter() to Config and a screen_width property, it will be better to use config.get_terminal_writer().fullwidth.

bluetech · 2020-06-15T17:19:09Z

src/_pytest/assertion/util.py

@@ -168,29 +171,7 @@ def assertrepr_compare(config, op: str, left: Any, right: Any) -> Optional[List[
    return [summary] + explanation


-def _compare_eq_any(left: Any, right: Any, verbose: int = 0) -> List[str]:


Just a note, moving code around makes it harder to review, because it breaks the diff view, so best to do it separately.

I guess this is a relevant comment for this PR 😄

bluetech · 2020-06-15T17:30:19Z

src/_pytest/assertion/util.py

+
+        def _text_header(header: str, screen_width: int, margin: int = 10) -> List[str]:
+            hlength = len(header)
+            lines = [
+                "=" * int((screen_width - hlength - margin) / 2)
+                + header
+                + "=" * int((screen_width - hlength - margin) / 2)
+            ]
+            if screen_width % 2 != 0:
+                lines[-1] += "="
+
+            return lines
+
+        explanation += _text_header(" ACTUAL ", screen_width)
+        explanation += list(left.split("\n"))
+        explanation += _text_header(" EXPECTED ", screen_width)
+        explanation += list(right.split("\n"))


IIUC str.center can take care of most of this. Also can you explain why the margin (10) is needed?

Suggested change

def _text_header(header: str, screen_width: int, margin: int = 10) -> List[str]:

hlength = len(header)

lines = [

"=" * int((screen_width - hlength - margin) / 2)

+ header

+ "=" * int((screen_width - hlength - margin) / 2)

]

if screen_width % 2 != 0:

lines[-1] += "="

return lines

explanation += _text_header(" ACTUAL ", screen_width)

explanation += list(left.split("\n"))

explanation += _text_header(" EXPECTED ", screen_width)

explanation += list(right.split("\n"))

explanation += [

" ACTUAL ".center(screen_width - 10, "="),

*left.split("\n"),

" EXPECTED ".center(screen_width - 10, "="),

*right.split("\n"),

]

gnikonorov · 2020-06-25T04:19:42Z

Hi @piotrhm . If this PR is meant to close an issue, could you please update the description as described here so that the linked issue may autoclose?

bluetech · 2020-08-14T07:12:53Z

Hi @piotrhm,

First of all we would like to thank you for your time and effort on working on this, the pytest team deeply appreciates it.

We noticed it has been awhile since you have updated this PR, however. pytest is a high activity project, with many issues/PRs being opened daily, so it is hard for us maintainers to track which PRs are ready for merging, for review, or need more attention.

So for those reasons we think it is best to close the PR for now, but with the only intention to cleanup our queue, it is by no means a rejection of your changes. We still encourage you to re-open this PR (it is just a click of a button away) when you are ready to get back to it.

Again we appreciate your time for working on this, and hope you might get back to this at a later time!

laurentbristiel reviewed Apr 19, 2020

View reviewed changes

changelog/6757.feature.rst Outdated Show resolved Hide resolved

bluetech reviewed Apr 19, 2020

View reviewed changes

nicoddemus requested changes May 2, 2020

View reviewed changes

nicoddemus approved these changes May 4, 2020

View reviewed changes

src/_pytest/assertion/util.py Show resolved Hide resolved

nicoddemus requested a review from bluetech June 2, 2020 15:18

piotrhm and others added 12 commits June 9, 2020 14:13

Change ._diff_text() function to handle long strings differently.

966c925

Update 6757.feature.rst

d345640

Added requested changes.

a79ad96

Fixed test_assertion.py

f281ef9

Fix linting.

d1e6a6f

Fix linting 2.

d91c19a

Fix linting 3.

dab79f3

Add test.

4a67892

Add test - fix linting.

4a9d91a

Add test - fix linting 2.

fd23a18

Add test - fix linting 3.

a607e7e

Fix lintign

93fba57

nicoddemus force-pushed the issue_6757 branch from aa0bd24 to 93fba57 Compare June 9, 2020 17:18

Fix static mypy check

41e87f5

piotrhm and others added 5 commits June 11, 2020 11:19

Merge branch 'master' into issue_6757

39b288b

Fixed merge conflict

d12ea82

Fixed merge conflict 2

526748f

Fixed screen_width

d48d895

Fix linting

0cdb2c8

bluetech requested changes Jun 15, 2020

View reviewed changes

bluetech closed this Aug 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 6757 - Improved diff output for similar strings. #7099

Issue 6757 - Improved diff output for similar strings. #7099

piotrhm commented Apr 19, 2020 •

edited

Loading

bluetech left a comment

bluetech commented Apr 19, 2020

nicoddemus left a comment

nicoddemus commented May 4, 2020 •

edited

Loading

nicoddemus commented Jun 9, 2020

nicoddemus commented Jun 10, 2020

piotrhm commented Jun 11, 2020

nicoddemus commented Jun 13, 2020

bluetech left a comment

bluetech Jun 15, 2020

bluetech Jun 15, 2020

bluetech Jun 15, 2020

gnikonorov commented Jun 25, 2020

bluetech commented Aug 14, 2020

		@@ -168,29 +171,7 @@ def assertrepr_compare(config, op: str, left: Any, right: Any) -> Optional[List[
		return [summary] + explanation


		def _compare_eq_any(left: Any, right: Any, verbose: int = 0) -> List[str]:

Issue 6757 - Improved diff output for similar strings. #7099

Issue 6757 - Improved diff output for similar strings. #7099

Conversation

piotrhm commented Apr 19, 2020 • edited Loading

bluetech left a comment

Choose a reason for hiding this comment

bluetech commented Apr 19, 2020

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus commented May 4, 2020 • edited Loading

nicoddemus commented Jun 9, 2020

nicoddemus commented Jun 10, 2020

piotrhm commented Jun 11, 2020

nicoddemus commented Jun 13, 2020

bluetech left a comment

Choose a reason for hiding this comment

bluetech Jun 15, 2020

Choose a reason for hiding this comment

bluetech Jun 15, 2020

Choose a reason for hiding this comment

bluetech Jun 15, 2020

Choose a reason for hiding this comment

gnikonorov commented Jun 25, 2020

bluetech commented Aug 14, 2020

piotrhm commented Apr 19, 2020 •

edited

Loading

nicoddemus commented May 4, 2020 •

edited

Loading