Conversation
saulecabrera
left a comment
There was a problem hiding this comment.
Thanks for fixing this! Left one comment around test coverage.
| } | ||
|
|
||
| #[test] | ||
| fn test_js_help() -> Result<()> { |
There was a problem hiding this comment.
Skimming this file it seems that we don't have tests for build -C help? Could you also add a test for that as well?
Additionally, could we add an integration test that exercises that error cases (i.e., plugins)
There was a problem hiding this comment.
I'll add one for build -C help but test_using_wasip1_plugin_with_static_build_fails_with_runtime_config and test_using_wasip2_plugin_with_static_build_fails_with_runtime_config exercise the error case where -J options are provided and a plugin is specified and ensures they fail with a helpful error message. Calling -J help while providing a plugin does succeed because as I mentioned in the PR description, there isn't a good way to have that error without adding a bunch of complexity around the input parameter needing to become optional to support that.
Description of the change
Move handling
-J helpinto the options parser instead of performing it later. This means we can inlineJsGroupOptionwhereverJsGroupValuewas used since we're not propagating that help was requested. Also update the error message emitted when someone tries to use-Jwith a plugin to mention it's not supported when specifying a plugin.Why am I making this change?
Fixes #1157.
I opted to move the handling with
-J helpinto the parser because it seems like makinginputoptional in this one particular case would introduce more complexity compared to just loading the default plugin to provide the runtime options. I updated the error message for using-Jwith plugins because it could be confusing for a user if the-J helpmessage says a particular option is supported and then they get an error message saying that option is not supported when trying to use it with a plugin.Checklist
javy-plugin-apiif the QuickJS bytecode has changed.javy-cli,javy-plugin, andjavy-plugin-processingdo not require updating CHANGELOG files.