campoy / embedmd

embedmd: embed code into markdown and keep everything in sync
Apache License 2.0
767 stars 62 forks source link

The command does not output the result #42

Closed rusbob closed 7 years ago

rusbob commented 7 years ago
  1. Install go:

    > go version
    version go1.8 windows/amd64
  2. Compile embedmd

    > go get github.com/campoy/embedmd
  3. Create folder and cope there two files docs.md & hello.go

  4. 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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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.

rusbob commented 7 years ago

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

dmitshur commented 7 years ago

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 commented 7 years ago

Ok, thanks you for details.