-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Format inserted/deleted maps in list assertions #13954
base: main
Are you sure you want to change the base?
Conversation
@sabiwara You're right, I was actually testing it with this code because the output of the test function was hard to read . When I swap the left and right it goes a different code path. Will investigate further 484 assert [map, %{map | hour: 8}] == [map] #this one is called
485 assert [map] == [map, %{map | hour: 8}] #this one is not |
@@ -469,6 +469,58 @@ defmodule ExUnit.FormatterTest do | |||
""" | |||
end | |||
|
|||
test "formats nested maps with column limit" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a test in diff_test.exs
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good should, updated
lib/ex_unit/lib/ex_unit/diff.ex
Outdated
end | ||
|
||
defp move_right({y, list1, [], edits}) do | ||
{y, list1, [], edits} | ||
end | ||
|
||
defp move_down({y, [elem1 | rest1], list2, {edit1, edit2, env}}) do | ||
{y + 1, rest1, list2, {[{:del, elem1} | edit1], edit2, env}} | ||
{y + 1, rest1, list2, {[{:del, build_elem(elem1)} | edit1], edit2, env}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am worried this change is happening too deep in the algorithm. We should probably deal with converting to struct/map when building the diff script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to move it where it is extracted from the diff later, it still may be not the best place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'ts enough to convert it in the escape
function, it is called in the diff_improper
when the non matched elem is inserted on the right (line 401) and list_script_to_diff
when it's in :del
on left side on line 621
I had to restrict the width from infinity to 80 chars to reproduce the error. |
@@ -126,10 +126,11 @@ defmodule ExUnit.DiffTest do | |||
assert env_binding == expected_binding | |||
end | |||
|
|||
@terminal_width 80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this changes other tests. I can either adjust it or make a custom helper to test it ???
Solves #13624 When the elements are equal then we pass them as ast, when deleted or inserted we pass maps as
%{a: 1, b: 2}
and fall into inspect in safe_to_algebra. This pr changes maps to ast representation so it's formatted correctly in the function that handles maps.I wrote a test, but I couldn't make it fail initially. However, it did fail when I added a regular assertion in my code. There are likely similar issues with other data structure, but now that I know what to change, it should be easier to extend the fix. It wasn't easy to find the issue with all the pattern matching in function heads and the single-element tuple which is not that easy to spot. It would be helpful to have a
dbg
implementation for multiple function heads.