Skip to content
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

Data Driven Model for Merge Tools #105

Open
jwillikers opened this issue Feb 21, 2020 · 1 comment
Open

Data Driven Model for Merge Tools #105

jwillikers opened this issue Feb 21, 2020 · 1 comment

Comments

@jwillikers
Copy link
Contributor

This is just an idea to make adding new merge tools simpler. A lot of the merge tools function quite similarly and their implementations in ApprovalTests each require the creation of at least on class per merge tool. However, given multiple versions, names, or operating systems, the number of classes required increases. This amounts to quite a bit of boilerplate when adding new merge tools. By defining a struct for storing all of the necessary data, construction of one-off classes can be avoided, and this struct can provide all of the necessary details required to find and call a diff tool from within ApprovalTests.

An example struct as follows hopefully encapsulates most of the variance between merge tools.

#include <string>
#include <vector>

enum Context {
  Background = 0;
  Foreground
};

struct DiffTool {
  std::string name;
  std::vector<std::string> search_paths;
  std::vector<std::string> names;
  std::string arguments;
  Context = Context::Background;
  unsigned int priority = 0;
};

By defining a standard search procedure for the search_paths and names, each tool can be found using the same process. The search_paths and names members would likely need to be defined in a way to only include certain values dependent on the OS.

I'm not sure how viable this is, but I wanted to get the idea out there. It would be nice to only have to define most merge tools in one place.

Note: Multiple versions of the same tool would still require special handling.

@isidore
Copy link
Member

isidore commented Aug 23, 2021

Hello,

Thanks for the suggestion.

I like the idea of being able to put multiple paths in a DiffInfo, but don't think we will do that until the next time we need to add a reporter that require this feature.

So good idea, but currently on hold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants