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

The command does not output the result #42

Closed
rusbob opened this issue Mar 1, 2017 · 7 comments
Closed

The command does not output the result #42

rusbob opened this issue Mar 1, 2017 · 7 comments

Comments

@rusbob
Copy link

rusbob commented Mar 1, 2017

  1. Install go:
> go version
version go1.8 windows/amd64
  1. Compile embedmd
> go get github.com/campoy/embedmd
  1. Create folder and cope there two files docs.md & hello.go

  2. Execute command to get result of output as written in README usages at https://github.com/campoy/embedmd:

> embedmd -d docs.md
docs.md:unexpected format for diff output:

> embedmd -w docs.md

Nothing is produced. No any result.md files. What could be wrong ?

@dmitshur
Copy link
Contributor

dmitshur commented Mar 1, 2017

Because you didn't include -u flag in go get, it's hard to know what version of embedmd you're using. It might be the latest, or it might be a really outdated one. 🤷‍♂️

Can you try again with -u flag:

go get -u github.com/campoy/embedmd

And see if the issue still occurs?

That said, it looks like there's something up with your diff command that embedmd uses to do a diff. You're using Windows, so it's quite likely.

What does diff --version print?

@rusbob
Copy link
Author

rusbob commented Mar 1, 2017

Ok, about code snippets generation it was my fault. I thought embedmd -w docs.md command creates a new file result.md, but it modified existing one i.e. docs.md

About diff. The issue is still present. The diff --version command not recognized in windows shell. I have only kdiff3.exe installed, but that is different than diff command.

@dmitshur
Copy link
Contributor

dmitshur commented Mar 1, 2017

About diff. The issue is still present. The diff --version command not recognized in windows shell. I have only kdiff3.exe installed, but that is different than diff command.

That's the cause of the problem. embedmd currently relies on an external diff command for its -d functionality, see main.go#L188.

@dmitshur
Copy link
Contributor

dmitshur commented Mar 1, 2017

@campoy Because the diff command is expected to exit with non-zero exit code when it successfully finds a difference, you should consider first checking that the command exists in PATH before trying to execute it. If it doesn't exist in path, you can print a more friendly error message (or do something else more involved).

if len(data) == 0 && err == nil {
	// diff exits with a non-zero status when the files don't match.
	// Ignore that failure as long as we get output.
	return nil, err
}

For example, see goimporters source for an example of checking that dot command exists before trying to rely on it:

func init() {
	if _, err := exec.LookPath("dot"); err != nil {
		// TODO: Replace dot with an importable native Go package to get rid of this annoying external dependency.
		fmt.Fprintln(os.Stderr, "`dot` command is required (try `brew install graphviz` to install it).")
		fmt.Fprintln(os.Stderr, err)
		os.Exit(1)
	}
}

(Source: https://github.com/shurcooL/cmd/blob/12e1fd290e879dec8ab9e88d830c9cd53c911c6a/goimporters/main.go#L21-L28.)

However, normally (at least on macOS), doing exec.Command("foo").CombinedOutput(), where foo is a command that doesn't exist in PATH, should return zero output and non-nil error "executable file not found in $PATH". I'm not sure why that didn't happen. It might be a Windows thing.

You might want to change main.go#L199 to use %q instead of %s so that invisible characters are more visible.

Based on the error message:

> embedmd -d docs.md
docs.md:unexpected format for diff output:

I'm guessing that the output was a single newline character, or something like that.

broady added a commit that referenced this issue Mar 1, 2017
@rusbob
Copy link
Author

rusbob commented Mar 2, 2017

The issue could be closed as the pull request is opened and will be merged to master soon.

@rusbob rusbob closed this as completed Mar 2, 2017
campoy pushed a commit that referenced this issue Mar 2, 2017
@dmitshur
Copy link
Contributor

dmitshur commented Mar 2, 2017

The issue could be closed as the pull request is opened and will be merged to master soon.

@rusbob, in general, it's a good practice to leave the issue open until the PR is actually merged and resolves the issue. That way, the issue status accurately represents the status of a feature/bug fix being complete at all times. Unexpected delays in the PR merging process can sometimes happen.

It's not a big deal in this case, I just wanted to let you know. :)

@rusbob
Copy link
Author

rusbob commented Mar 3, 2017

Ok, thanks you for details.

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

No branches or pull requests

2 participants