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

Add a command-line interface to the WML diff functionality #3721

Open
wants to merge 3 commits into
base: master
from

Conversation

@CelticMinstrel
Copy link
Member

commented Nov 14, 2018

The diff functionality is there, so I figured it could be useful to expose it.

I would've just pushed straight to master instead of making a pull request, but I noticed output redirection doesn't seem to be working, so I decided to see if anyone had any idea why.

To be precise, the following command:

wesnoth.exe --diff test1.cfg test2.cfg > diff.cfg

outputs the diff to the console and not to diff.cfg. I tried both in VSCode's integrated terminal and in a standard Windows command prompt. I imagine it'll probably work as expected on Linux, but I'd like it to work on Windows too.

.BI -D,--diff \ left-file \ right-file
diffs the two WML files; does not preprocess them first (to do that, run them through
.B -p
first. Outputs the diff as DiffWML on standard output.

This comment has been minimized.

Copy link
@jostephd

jostephd Nov 14, 2018

Member
Suggested change
first. Outputs the diff as DiffWML on standard output.
first). Outputs the diff as DiffWML on standard output.
@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

is there a usecase for this?

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

So, shadowm tells me that the output redirection issue is something that's unavoidable on Windows due to the way Wesnoth sets up its console redirection (unless we were to offer a separate Wesnoth executable for command-line use, maybe). Thus, I'm going to have to rethink this implementation, as the output file will need to be passed to Wesnoth rather than making use of shell redirection if this is to be useable on Windows.

I could either:

  • Make the --diff and --patch flags require three arguments rather than two.
  • Add an additional flag to specify the output file.

Any thoughts?

As for a use-case... I think there could be a use-case but I've been unable to come up with a concrete example. This functionality is used in the engine, but for typical WML files a text-based diff is probably more useful than this structural diff.

Maybe this could be useful for comparing different unit type definitions? A text-based diff would be sensitive to the order of keys, but if I understand correctly, the structural diff would not be. I would need to enable preprocessing in the diff function for that to be possible though...

@jostephd

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

I think I prefer an additional flag to specify the output file. A --diff option that takes "left" and "right" arguments is intuitive; one that takes three arguments is not.

@CelticMinstrel CelticMinstrel force-pushed the wml_diff_cl branch from ab7b495 to ba9d1c3 Mar 10, 2019

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2019

I'd like to also make the --output flag work for the preprocess options, but it's a bit complicated; they actually output three files, so some overhaul of the preprocess options might be required for this to work.

std::ifstream in_base(cmdline_opts.diff_left);
std::ifstream in_diff(cmdline_opts.diff_right);
read(base, in_base);
read(diff, in_diff);
config left, right;
std::ifstream in_left(cmdline_opts.diff_left);
std::ifstream in_right(cmdline_opts.diff_right);
read(left, in_left);
config base, diff;
std::ifstream in_base(cmdline_opts.diff_left);
std::ifstream in_diff(cmdline_opts.diff_right);
read(base, in_base);
std::ifstream in_left(cmdline_opts.diff_left);
std::ifstream in_right(cmdline_opts.diff_right);
read(left, in_left);
read(right, in_right);
@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2019

(What the heck is codacy on about...?)

@jostephd

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

I think it might be confusing our read() overload with the read() syscall.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.