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

WireViz 0.4 changed CLI parameters #5

Closed
martinrieder opened this issue May 13, 2024 · 12 comments · Fixed by #11
Closed

WireViz 0.4 changed CLI parameters #5

martinrieder opened this issue May 13, 2024 · 12 comments · Fixed by #11
Labels
enhancement New feature or request
Milestone

Comments

@martinrieder
Copy link

martinrieder commented May 13, 2024

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
Copy link
Owner

nanangp commented May 13, 2024

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 nanangp added enhancement New feature or request blocked Blocked by external issues labels May 16, 2024
@nanangp
Copy link
Owner

nanangp commented May 16, 2024

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 nanangp added this to the v1.1.0 milestone May 16, 2024
@nanangp
Copy link
Owner

nanangp commented May 16, 2024

@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
Copy link
Author

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

@nanangp
Copy link
Owner

nanangp commented May 16, 2024

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

@martinrieder
Copy link
Author

martinrieder commented May 16, 2024

@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
Copy link
Author

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
Copy link
Owner

nanangp commented May 16, 2024

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
Copy link
Author

martinrieder commented May 16, 2024

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 wireviz/WireViz#320 (comment)

nanangp pushed a commit that referenced this issue May 20, 2024
* [fix] Prevent errors caused by concurrent wireviz invocation (#8)
* [new] Notify users running older version of WireViz (#6)
* [new] Also log warnings/errors to an output channel (#5)
@nanangp nanangp removed the blocked Blocked by external issues label May 21, 2024
@martinrieder
Copy link
Author

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

@martinrieder
Copy link
Author

martinrieder commented May 21, 2024

Reopened because I found an absolute path in .vscode/launch.json from the last commit 3c5ebbc 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
Copy link
Owner

nanangp commented May 21, 2024

absolute path

Oops

@martinrieder martinrieder changed the title WireViz 0.4 changing commandline options and output formats WireViz 0.4 changed CLI parameters May 22, 2024
@nanangp nanangp linked a pull request May 24, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants