Fix difftools args #593
Fix difftools args #593
Conversation
|
@SimonCropp would you please have a glance and maybe pull these changes? There's an issue in statements order on CI. Is that ok? |
| *.received.txt | ||
| src/TestResults/ | ||
| src/TestDiffTools/Main.approved.txt | ||
| src/TestDiffTools/*.approved.* |
There was a problem hiding this comment.
We don't want to ignore these as it's good to have them visible in the PR to identity what's a breaking change.
There was a problem hiding this comment.
Do we still need Main.approved ignored? This is the only approval file in the utility for testing purposes.
|
i have been holding off on this one, since i would prefer to consume https://github.com/VerifyTests/DiffEngine/ in shouldly. @josephwoodward thoughts? |
| { | ||
| public VimDiff() : base("VimDiff", new DiffToolConfig | ||
| { | ||
| WindowsPath = @"vim.bat", |
There was a problem hiding this comment.
is this really safe to assume on all machines?
There was a problem hiding this comment.
Typical vim for windows installer puts several *.bat files into %WINDIR% folder when option Create .bat files selected. Search for editor in %PATH% seemed more reasonable to me. What do you think?
|
@pavel-usachev some minor feedback on the PR as a whole. when you do a PR, try and keep it as small and isolated as possible. In this PR you have fixed a bug, added a feature, and done a non trivial refactor. Combining all these things will significantly slow down the progress of the PR |
|
@SimonCropp thanks for your advice. Next time I'll think twice. |
|
fixed by #644 |
Fixes #592
Added vimdiff tool.