-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Use DiffPlex to produce better diffs #346
Conversation
@drhumlen Thank you for your suggestion and contribution! I hope you stick around and do more great stuff like this. I really like the improved outputs, but I don't like taking on new dependencies. I see two alternatives;
|
Yeah, I understand that. It's always good to keep dependencies to a minimum to avoid being ... well dependent. I was actually thinking about making the diffing function a user-configurable thing – so you can pass whichever differ function you want to use to Expecto. That way, you as a user can opt in on DiffPlex – or some other diffing library if you want to. Porting it is also interesting. At first I thought this diffing algorithm was very complex, but it doesn't look too daunting looking at the code you linked. Perhaps just porting it over is the easiest and best result? 🤔 It's possible to do both also. Make the differ a configuration for the expecto user and port over the algorithm as a "better default" (perhaps with the option to use the current differ). |
It's also possible to add a gutter with linenumber & color coding the change: blue=modified, green=whole line added, red=whole line deleted, yellow="imaginary" – just like many git tools support. Not sure if it's too much with numbers or not 🤔perhaps make it configurable? And of course don't display line numbers when there's only one line. If this feature is worth having, it also needs to be part of the abstraction. I think it's pretty cool, but perhaps highlighting words will suffice? |
This is not really necessary if we make it good by default; sometimes choice is a curse, too, and I've never seen any other testing library that lets the user change the diff function; or even seen anybody asking for that feature in the other libs I've used.
Well, you'd have to port those 400-something LoC, but you could run the same tests that are already written in C# against your implementation, so you wouldn't have to port those. It would probably take an hour or four to port the code. It's probably a fun exercise and it would drastically improve the stability of this code base, compared to taking a dependency.
I'll let you decide how it should look for now; what you've done is much better than what we already have. |
As for building with net461/mono; you first install mono, and then do |
Yea, after reading some more through the library, it seems my initial gut-feeling was more right. There is significant work done by the DiffPlex, and it would be almost a little silly to rewrite all of that in F# – even if parts of it is trivial conversion. What if we just grabbed the C# code directly and places it inside this repo instead of converting it to F#? I could go ahead and delete those (few) files we don't need to keep it small/simple. No reason to re-invent the wheel..? I guess it's possible to rewrite it in F#, but it's tedious work... It's either that, or making improved diffs an addon with its own repo. But I agree with you, maybe that's making it too flexible, and also a bit harder for a newcomer to have to add several packages just to get a nice diff. |
Sure, let's go for this then; but then it would be a dependency named |
Hi there, I am curious why the hesitancy of taking one dependency? You are free to copy the code, but it is often nicer for the community to take a lib dependency and the contribute changes and improvements back as you find things to enhance. |
@mmanela I'm sure we'll repatriate code if we find improvements. Why not a dependency: I've taken "one dependency" before in Suave's source code, on FsPickler, believing it would be semver and schema compatible in minor releases, but it turned out it was not. The author of FsPickler thought this was fine, and we had to do-it-ourselves. The exact same happened for Argu in this lib, which caused us to build our own argument parser in this library. Logging; we're importing my Logary Facade lib, so that we can freeze its API, and Logary itself has a large test suite to avoid ABI incompatibilities and make sure Logary.Facade.Adapter doesn't go out of sync. There's an explicit version field in the source code, too. In this case, looking at the list of releases https://www.nuget.org/packages/DiffPlex/ there's not a single major version bump, which means that you've either kept the ABI/API breaking changes to zero the last 8 years, or there have been breaking changes that haven't been versioned as such. I haven't analysed the repository though. Further, as this project is integrated into FAKE, Rider and VSCode, I think they deserve a stable API. Taking a dependency also means we'll have to @drhumlen Thinking like above, perhaps we'll just have a 'sattelite' assembly and reference DiffPlex from it, like we do with Hopac; however, we'd still need the simple integration to happen once it's referenced. |
I understand and at least for diffplex, you are right I have kept the api very stable since there is not much need to change it |
@haf: Yeah, I'm trying to figure out the best way to do it now. On the top of my head, we could introduce a new function in Expect.fs:
and then let users import
and then go on with their tests:
instead of the usual ... But that doesn't feel quite optimal if the rest of your tests starts with Expect._____ Alternatively, we could make Expect more of a class
and then let users do open Expecto let Expect = Expecto(betterDiffPrinter) test "This is my test" { That also opens up rooms for inserting more user-configurable things into the Expect-module in the future. But then again, I do like how the current Expect-module works without the need for instantiating a class. Hmm... |
A bit off-topic: When writing my own tests, I find that Expect.equal is the function is use by far the most. I also rather use more time to describe the test-case well instead of using the message-field – it's almost always redundant. I rather spend more time perfecting the name of helper functions and local values to make the test understandable. ...Which eventually led to me introduce a custom operator for assertions in my own test code base: let (==>) actual expected = Expect.equal actual expected "" // <- I don't need this message field Then I can write my tests simply like so:
(We're doing event sourcing, so we have a lot of tests for our reducers (event appliers) & command handlers.) I find this way of writing asserts lets me drop a lot of parentheses, while also being very visually guiding with the "arrow syntax". I initially had many tests written very nestedly with Expect.equal (......) (..........) "", but I've slowly rewritten many tests to use the arrow and found that they become a more readable. The reasons I'm mentioning it is because I think the Expect.equal function is by far the most useful one. You could do almost all the others things with it: I'm not sure if it's wise to pollute the namespace with a lot of symbolic operators, but perhaps one is not too unreasonable – for example Just thougth I'd put it out there. Maybe this belongs in its own thread though. |
On-topic again: the easiest would be to just depend on DiffPlex (like i've done initially) and not bother with all the hassle of either porting code over to F# or confusing the end user with a plugin-like system. ...And if DiffPlex's API turns out to be unstable, we can always just go back to the old differ, or copy over the code that worked into this repo. Also, as long as we depend on a specific version of DiffPlex, I don't see why it should ever stop working? Also, @mmanela himself confirms that DiffPlex's api has been very stable and will probably continue to be since there's not much need to change it (ever)? |
Ok, I just went ahead trying out the Expecto.Hopac approach – making a seperate module and keeping Expecto (core) depedency-free. I'm new to paket, and had to do a lot of wrestling with my IDE and Paket along the way, so some of the Paket-code/dependency tree maybe unnecessary/wrong. Please give feedback if there's something strange there. Since the Colorization code is an internal/private module for Expecto, I have to pass a colorization function to the Expecto.Differ module's function – which looks very strange, but is is necessary to keep the color scheming consistent I think? I'm still not quite done yet. String diffing doesn't use the provided differ function in every case, so that needs to be straightened out. But I'm awaiting feedback from you for now in case you've changed your mind on the dependency situation, or have a better solution :) |
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.
OK, it's starting to look good. I just think we need to design the surface area better; e.g. colouriseString
should not be a public function and I'd prefer if Expecto
's library knows about Expecto.Differ
, in that it automatically configures it if it's available.
How do to that;
- lazily upon diffing, check if we can load
Expecto.Differ
and if so, replace the current diffing function with the differ function - OR; we shadow the functions that might want to perform diffs; the problem of course being that there's a large number of such functions
As for the colours, I think the DVar approach is a nice one.
Line 377 in 0b2bca6
module DVar = |
Also, the name; Differ
sounds a bit strange to me. Perhaps we can name it plainly Expecto.Diff
?
Expecto.Differ/Expecto.Differ.fsproj
Outdated
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks> | ||
<GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
<!-- Optional: Declare that the Repository URL can be published to NuSpec --> | ||
<PublishRepositoryUrl>true</PublishRepositoryUrl> |
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 haven't seen this before? What's its effect?
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 just copied from Expecto.Halfo. I know almost nothing about these XML-files; i just really want them to work on my machine and for my IDE to recognize them :) I'm basicly banging rocks together hoping for the depedencies to resolve.
Expecto.Differ/Library.fs
Outdated
open DiffPlex.DiffBuilder | ||
open DiffPlex.DiffBuilder.Model | ||
|
||
let colorizedDiff (colorizeText: ConsoleColor -> string -> string) first second = |
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.
We use en-GB
in this repo, so colourisedDiff
, if you please ;)
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.
Let's chat about the colourisation logic soon.
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.
Alright, consistency is key :)
Even though I'm a little confused by the choice of using "colour" etc. when the rest of the industry uses "color" – we're even using the ConsoleColor type here. But at the end of the day it doesn't matter of course as long as it's consistent. I'll fix!
Expecto.Differ/Library.fs
Outdated
| (:? string as f), (:? string as s) -> | ||
string f, string s | ||
| f, s -> | ||
sprintf "%A" f, sprintf "%A" s |
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 will generate different indentation depending on the values of the fields in the objects. Of course, this is still an improvement, but a tree-walking algo that does consistent indentation would be preferred.
Expecto.Differ/Library.fs
Outdated
|
||
sprintf "\n---------- Actual: --------------------\n%s\n---------- Expected: ------------------\n%s\n" (colorizedDiff diff.OldText.Lines) (colorizedDiff diff.NewText.Lines) | ||
|
||
let equals actual expected message = |
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 see; so you're thinking we open Expecto.Differ
and this symbol shadows Expect.equal
? What about sequenceEquals
and the ilk?
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.
Actually I wasn't thinking about shadowing at all ... But shadowing is a possibility I guess 🤔
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.
open Expecto.Flip
does exactly that, if you want an example ;)
Expecto.Sample/Expecto.Sample.fs
Outdated
let colorizationTests = | ||
testList "Colorization tests. These should all fail to diplay the diff" [ | ||
test "Testing diff colorization" { | ||
let personA = {Name = "Kesam"; Age = 30} |
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.
Can we also have a test for a huge object where the fields differ a lot?
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.
Absolutely. I just used tiny differences because those are the ones I'm usually struggling with spotting when the object is big and the diff colorization fails me.
I'll add a bigger object where many of the fields are wrong? Do you have an example in mind? Like {Address, Name, City, SecurityCard={Number, Expiration, CCV}, Pet=[{PetName=, PetSpecie}...]} ish with many fields differentiating?
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.
No, it's enough if a field deep into a list of items has both long values in its fields and one of those fields differ. What I'm trying to trigger is that %A
formats newlines if it gets to large fields.
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.
Ah yeah i thought so. Imo it works satisfactory now, but perhaps you want to checkout the branch and play with a diff like that yourself?
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.
Ah yeah i thought so. Imo it works satisfactory now, but perhaps you want to checkout the branch and play with a diff like that yourself?
BTW; DVar https://github.com/haf/DVar/blob/master/src/DVar.Tests/Program.fs — it's also present in Logary (and its Facade in this repo) |
Looks fine to me! |
Thanks for the feedback. I'll refactor my code. It was never meant to be in a finished state; I just needed some feedback on the general direction before putting too much time into it. I'm still the most concerned with: 1) How the user will "plugin" this and have it work without having to do anything weird. 2) How to pass the colourisation function to the new plugin. The way I've done it now works, but it's not too pretty 🤔 DVar is new to me, but looks interesting |
…n needed to colorise text for plugins
…iffer as un-intrusive as possible
Here I use a mutable variable to set the defaultDiffPrinter. I'm usually against mutable variables, but this gives us the un-intrusive plugin experience with only a single small mutable variable. Only catch is the user have to remember to set the |
...Or, I guess the Expecto.Diff could just have the side-effect when running. But that's really dark magic: An import that modifies the program..? |
@haf : Here's a big object diff |
Feel free to checkout the branch and do the necessary steps to make this become mergeable. I'm not sure how to use DVar, or check if a module is available/loadable like you described. But I think the mutable variable does a pretty good job; it's just a hassle to remember to set it before running one's tests. |
@haf : What will it take to get this merged now? The only thing that I feel is a bit "unsolved" now is the use of a variable to switch the differ function. It's possible to use a Regarding tree-walking records, I think that's not really needed when you have a really good diff. You immediately see what the diff is, and the whole diff, so ... is it needed? Tree-walking also only works for records afaik, so if you have an seq of records, or a record that contains a seq, then it won't help you anyway. If I could wish for one thing (from F# in general?), it would be better pretty-printing. The default, %A, could've been so much nicer without the bloated semicolons and sometimes weird array indentation. |
This is pretty sweet and I'm going to steal this for my Xunit asserts. |
@drhumlen No there's just the coordination of the config; we can merge but then I lock down master for releases until we have a nicer public reconfiguration API... Are you sure you don't want to take a stab at solving the DVar integration? :) There are examples of its usages in Logging.fs |
🙌 @drhumlen And thank you very much for the contribution :) |
Wow, this sounds amazing! I’m so happy I found this, and a bit sad I didn’t find it sooner. Would you mind explaining how to enable this? I can’t figure it out. 🙏 @jzabroski Did you end up doing something similar for Xunit? I’d love to steal that for projects not using Expecto 😄 |
@lydell there's a code sample here in this PR demonstrating it, but you have to
|
@baronfel Thank you so much! 🥇 Also, I tried using |
Hi! I've long been wishing for better diffs when comparing bigger objects/records or strings. I've searched around for a library to do better string diffing, and found a library called DiffPlex that does just that. I've posted some example screenshot below of the current diff rendering vs DiffPlex's. Imo it becomes a lot easier to see exactly what is changed in bigger object.
I've added some failing tests to the Sample project (not sure if it's the right place or not) to illustrate the new rendering.
This is still work in progress, but I rather show it off now before I put more time into it in case there's some reasons we can't do it this way. (Perfomance, compatability, preference..?). (Note also that I temporarily removed net461 because I couldn't get net461 to run on my machine before my patience ran out. (I'm on MacOS and every installer from microsoft seems to be .exe files))
What do you think?
Rendering examples: