nanangp / vscode-wireviz-preview

A simple extension for Visual Studio Code to preview WireViz YAML files
https://marketplace.visualstudio.com/items?itemName=NanangP.vscode-wireviz-preview
GNU General Public License v3.0
5 stars 1 forks source link

WireViz 0.4 changed CLI parameters #5

Closed martinrieder closed 3 months ago

martinrieder commented 3 months ago

A WireViz "release candidate" release/v0.4-rc2 was merged into master based on PR wireviz/WireViz#339. See the change log here for details about new command line options and output file formats. Note that also examples 11-14 were added to demonstrate the new features.

The VSCode extension is still able to run WireViz 0.4 and show an output image. However, it would not show an error messages to the user when it fails and therefore no files were generated. Additionally, if the picture already existed from a previously successful run, this would be presented to the user instead without any error indication.

I am pasting the --help message here for reference:

Usage: wireviz [OPTIONS] [FILE]...

  Parses the provided FILE and generates the specified outputs.

Options:
  -f, --format TEXT       Output formats (see below).  [default: hpst]
  -p, --prepend PATH      YAML file to prepend to the input file (optional).
  -o, --output-dir PATH   Directory to use for output files, if different from
                          input file directory.
  -O, --output-name TEXT  File name (without extension) to use for output
                          files, if different from input file name.
  -V, --version           Output WireViz version and exit.
  --help                  Show this message and exit.

  The -f or --format option accepts a string containing one or more of the
  following characters to specify which file types to output: c (CSV), g (GV),
  h (HTML), p (PNG), P (PDF), s (SVG), t (TSV)
nanangp commented 3 months ago

Oh nice. Looking at the changelog, seems like this should resolve some of the clutter that resulted in workarounds for #3.

I'll also have a look at why errors are being swallowed. Changelog didn't seem to mention anything.

PS. Sorry turnaround may be a little slow on this. I've got other stuff on my plate at the moment. If we're lucky, we may have something by the time 0.4.reaches RTM. Woops, it's RTM already.

nanangp commented 3 months ago

Currently blocked by wireviz/WireViz#344 on 0.4, so I couldn't test anything. Will probably hold off until their proposed fix (planned for 0.4.1) has been properly tested and released.

nanangp commented 3 months ago

@martinrieder would you be willing to test the wireviz0.4 branch of this extension?

I've made it work with their proposed fix for the aforementioned issue, but I'd like it for someone running an official 0.4 to confirm.

martinrieder commented 3 months ago

Would it be sufficient to only replace the files for testing? Or do I need to reinstall the extension?

nanangp commented 3 months ago

If you're running the extension from source, you could just git pull/checkout the whole branch and run it.

martinrieder commented 3 months ago

@nanangp Thanks a lot for this quick reaction.

No, I had it installed from Marketplace, so I did the git checkout and npm run build separately, then copied over the extension.js file.

I can confirm that it executes, though there are some timing issues. When VScode has not finished saving the file or you double click the Preview (F8) button, you might see an Error shortly before it displays an image. I have not been able to capture a screenshot of it yet.

Besides these glitches, I am quite happy about your changes. Pictures are generated for both PNG and SVG, even with included images. These can also be placed in another folder!

martinrieder commented 3 months ago

Could you possibly feed to the output messages to the console instead of the panel? Or STDOUT to the console and STDERR to the panel?

nanangp commented 3 months ago

though there are some timing issues.

That's interesting. It's possible that process.spawn hasn't finished when the extension is triggered the second time.

This may have been an existing bug, or a side effect of the small change to using the process' close event instead of exit. I'll see if this is a quick fix. If not, we'll open a new issue and fix it later.

Could you possibly feed to the output messages to the console instead of the panel?

Sounds good. I'll add debug console logging in addition to the panel.

It's rather annoying that WV is using stdout instead of stderr for a bunch of their errors. This was meant to be fixed in the stdout PR that is yet to be merged.

martinrieder commented 3 months ago

Note that wireviz/WireViz#321 by @ggrossetie resolves wireviz/WireViz#320, but is not yet part of the 0.4 release.

I agree that when "-" as input file should be interpreted as STDIN, then "-" as output file similarly should be interpreted as STDOUT. The output filename stem is by default equal to the input filename stem unless specified differently using -o.

I'm not sure if it makes sense to output more than one file format to STDOUT, but try it out to see if it can be usable.

There are a few places in the code where some warning text is written to STDOUT. Please make sure they are all written to STDERR to avoid possible conflicts with an output file written to STDOUT.

@kvid in https://github.com/wireviz/WireViz/issues/320#issuecomment-1635069465

martinrieder commented 3 months ago

Closing this, as we have #8 to follow up.

martinrieder commented 3 months ago

Reopened because I found an absolute path in .vscode/launch.json from the last commit https://github.com/nanangp/vscode-wireviz-preview/commit/3c5ebbc4125e9bc0c2678c339e81c37baf5f1ba8 by @nanangp I am not sure what the impact of this is, but it doesn't look correct.

Having the --prepend option implemented is not mandatory for closing this issue. The inclusion of files is WIP anyway, so it might be removed in future releases.

I created #9 feature request for output file formats though, which extends the current funionality.

nanangp commented 3 months ago

absolute path

Oops