Format pattern diff #8440
Format pattern diff #8440
Conversation
…istance of a pattern in an assert test
…urns an ExUnit.Pattern
| defp compare_list(%{ast: [{:|, _, [head, tail]} | _rest]}, [rh_head | rh_tail], env) do | ||
| head_pattern = %{ast: head, type: :cons_l} | ||
| tail_pattern = %{ast: tail, type: :cons_r} | ||
| {h, env} = compare(head_pattern, rh_head, env) |
There was a problem hiding this comment.
The result using Myers difference is much better than comparing heads, especially with one wrong item at the beginning of the list. The problem is hard to use it if you have things like | operator inside of the list.
I would suggest to parse the list before and make it flat, saving the indexes were the pipes occur so you can rebuild the pipes into the result. You will have another problem here because your %PatternDiff{} ties the left and the right sides together, so it's hard to know which index you are actually in. I would change the struct to represent only one side and return two structs, one for the left (pattern) and other for the right (value), then it will be really easy to find the indexes and also it will be easy to print the differences.
If you do that, you can basically copy the Myers algorithm from the List module and change only the comparison to check that two elements are equal, the compact_reverse/2 to rebuild the list and the accumulator to have all the :ins and :eq on the right side and :del and :eq (again) on the left side.
There was a problem hiding this comment.
You will also notice that you won't need anymore the @no_value because you no longer have to the difference between the two sides, making the diffs easier to understand.
There was a problem hiding this comment.
@josevalim I don't know if it's the case, but I think it would be amazing if the List.myers_difference could also accept a comparison function since it's now accepting a diff one.
| |> Enum.intersperse(", ") | ||
|
|
||
| [ | ||
| ["%{" | items] ++ ["}"] |
There was a problem hiding this comment.
I believe you can use the Inspect.Algebra module and just use Inspect.Algebra.format/2 to print after. It will make the formatting way more flexible and print nicely the things that don't fit on one line. If I'm not wrong this was also suggested in the original issue.
| @@ -0,0 +1,265 @@ | |||
| defmodule ExUnit.Pattern.FormatValue do | |||
There was a problem hiding this comment.
This module is very similar to ExUnit.Pattern.PatternValue and can also be completely merged together if you represent the two diffs separately and with the same internal structure.
| @no_value :ex_unit_no_meaningful_value | ||
|
|
||
| def create_context(%ContainerDiff{type: :list, items: items}, old_ctx) do | ||
| if Enum.all?(items, &keyword_tuple?/1), |
There was a problem hiding this comment.
You can move this logic to the format because this only changes the presentation.
| defmodule ExUnit.Pattern.DiffContext do | ||
| alias ExUnit.{ContainerDiff, PatternDiff} | ||
|
|
||
| defstruct [:keys, :print_when?, :vars, :pins] |
There was a problem hiding this comment.
The attribute print_when? can also be dropped if you have two separated diffs because you will only have when when they actually exists and should be printed
|
Hi @jeremyowensboggs! How are you? Just a follow up here: what do you think are the next steps? Do you want us to work on @ggcampinho's feedback and tidy up the work? Or will you plan to incorporate the proposed changes? Thanks! |
|
Hi,
If someone else could take over, that would be great?
Thanks,
Jeremy
On Jan 25, 2019, at 5:11 AM, José Valim <notifications@github.com<mailto:notifications@github.com>> wrote:
Hi @jeremyowensboggs<https://github.com/jeremyowensboggs>! How are you?
Just a follow up here: what do you think are the next steps? Do you want us to work on @ggcampinho<https://github.com/ggcampinho>'s feedback and tidy up the work? Or will you plan to incorporate the proposed changes?
Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#8440 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AX8STwbxsS7kMSLuf5MWE_0wbThTXnzcks5vGuZSgaJpZM4Y72jf>.
The information contained in this message is confidential information and privileged. It is intended only for the use of the individual or individuals or entity identified above. You are hereby notified that any dissemination, distribution, use or copying of this message is strictly prohibited.
If the receiver of this message is not the intended recipient, you are hereby notified that any dissemination, distribution, use or copying of this message is strictly prohibited. If you have received this message in error, please immediately notify the sender by replying to his/her e-mail address noted above and delete the message. Thank You
|
|
Thanks for the prompt reply @jeremyowensboggs! @ggcampinho would you like to do a pass on it incorporating your suggestions before we go ahead? |
|
I will work on it, I send a new PR soon with the changes
…On Sat, Jan 26, 2019, 09:40 José Valim ***@***.*** wrote:
Thanks for the prompt reply @jeremyowensboggs
<https://github.com/jeremyowensboggs>!
@ggcampinho <https://github.com/ggcampinho> would you like to do a pass
on it incorporating your suggestions before we go ahead?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8440 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACt1pnA-J_o6bNOpjNzT3rxuiwXAJ0Vhks5vHBSBgaJpZM4Y72jf>
.
|
|
Sorry, I had some personal issues and I ended up delaying this. I already checked how to integrate my code to this PR and I'll be working on this next week |
4 participants
Marks the conflicting parts of the pattern in red.
For assert_receive, prints the pattern & the value for each of the last 10 messages, coloring the conflicting parts of the pattern.
I updated the value formatting module as well, but left it disabled for now.