-
Notifications
You must be signed in to change notification settings - Fork 145
Fix final layer of error handling in command plugin #795
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
Fix final layer of error handling in command plugin #795
Conversation
//cc @MaxDesiatov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually output the validation error? I don't see that changed in the diff, but I'm not familiar with the code to say for sure
@MaxDesiatov the validation output was always printed (it should have been in your output already, just way up because of the stderr/stdout flushing differences). You can see in the examples in the PR description that they were printed in both cases too. Now they'll be printed in the correct order and you'll see them right before the final error. Would you mind running your repro against this branch to confirm? |
I see, yes it's at the top, which I personally find unobvious. May I suggest that separating the error messages to be placed on adjacent lines? |
@MaxDesiatov I'm just wanting to confirm things here... your comment on this PR says:
To be clear, we have solved exactly that in this PR. The weird ordering was the use of both stdout and stderr. It should now be the case that the final output of the tool in this failure mode is an error that it couldn't generate and, right above that, will be the errors. But... in your comment on the issue you say
I'm pretty sure that this PR addresses your issue but I'd like to make sure I haven't missed something before we cut a release 🙏 |
Sorry, the error message was there, but it was cut off because my terminal height was set to just a few lines. I didn't notice it because of the ordering. If changing from stdout to stderr fixes the ordering issue, that's much appreciated, thanks! |
if error.isMisconfigurationError { | ||
print("- OpenAPI code generation failed with error.") | ||
print("- Stopping because target isn't configured for OpenAPI code generation.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not quite right though.
When isMisconfigurationError
is true
, It's pretty much certain that the user configured their target for openapi, but did it wrong.
When isMisconfigurationError
is false
, it can be either a misconfiguration or no-configuration.
Or at least that's what I intended with this code, IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not defending the existing message and handling. Just saying the new message/handling can also be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, how about we just tweak this one?
if error.isMisconfigurationError { | |
print("- OpenAPI code generation failed with error.") | |
print("- Stopping because target isn't configured for OpenAPI code generation.") | |
if error.isMisconfigurationError { | |
print("- Stopping because target is misconfigured for OpenAPI code generation.") |
//cc @MahdiBM @czechboy0
…et argument (#798) ### Motivation In #795 we adjusted the logic of the command plugin because errors were not propagating properly. As a result, we regressed behaviour when the command plugin was run without any `--target` argument. In this mode, the plugin runs on all targets. When it detects a misconfigured target it errors, as it should. However, the classification of misconfigured target included targets that were not intended for code generation at all, resulting in errors running the command plugin on packages with others targets. ### Modifications - Add command plugin generation step to integration test - If the command plugin is running without a `--target` argument and cannot find _any_ of the required files, skip that target. ### Result - Manual code generation with the command plugin fixed for packages with other targets. - Fixes #797. ### Test Plan This PR includes two commits. The first adds a step to the integration test, which runs the command plugin on the integration test package. This will fail in CI. Then the second commit will provide the fix, which should pass in CI. The commits will then be squash merged.
Motivation
When the generator is run as a command plugin on a correctly configured target but there are problems generating, there's a layer of error handling which is incorrect resulting in a poor user experience. For example, if the OpenAPI document contains a bad type like this...
...then the command plugin shows the following output:
There are two issues with this:
Modifications
Result
The above example now results in:
Related issues
openapi.yaml
produces unexpected diagnostic message #794