Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

more information when swagger error vs warning, much easier to debug #43 #44

Closed
wants to merge 1 commit into from

Conversation

avishaan
Copy link

This is my suggestion for #43 When there is an error in the swagger doc just give us the whole error object. I think that would help people just getting started with your library. Myself included.

@whitlockjc
Copy link
Member

I don't think this is a good idea as-is and let me explain why. There are three parts of the error/warning that should always be short (reasonably at least):

  • code: A machine id for the error
  • message: The human readable explanation of the error (Errors from swagger-tools should be intuitive but I cannot guarantee the same thing from structural errors like you're seeing as they come from another library)
  • path: The array of object properties/indices to get you to the offending value

swagger-tools is already printing the information that has real value to them, message and path (As a JSON Pointer string) and code isn't that useful in my opinion. The 4th attribute, data, is a value that in most cases will not be a small value. I realize in the scenario that led to this, the data attribute would be 2.0, short and simple. But many of the errors/warnings can have full blown JavaScript objects as the data value which would not print very well. (Imagine having a full model definition or parameter definition being printed to the console.)

The purpose of the CLI is to give a pointer/reference to where the problem lies in your document and a human readable error message that explains the problem, much like other CLI tools do. When I designed this, I looked at existing tools like JSHint to see how they printed their output. One thing you might not be aware of is that there is already a way to invoke the CLI to get the full JSON representation of the validation errors. If you pass the --json flag to the CLI, you will get the full JSON payload of the errors/warnings.

With the --json flag and the things I've described above, where do you stand now? I'm mentioning @theganyo to get his opinion as well.

@theganyo
Copy link
Contributor

I agree, @whitlockjc. I think there should be a compact representation by default and what exists provides enough of a pointer to figure out where to look. Showing the data would generally be redundant - as you'll see the data as soon as you look at the element anyway to figure out the issue. But if you really need it, as you said, there's the --json flag...

@avishaan
Copy link
Author

Makes sense. I was thinking from a beginner's perspective where they may be very unfamiliar and the more verbose the error the better the ability to diagnose the problem which means the better adoption of a relatively new library. That is an example that seems to be tailored to a first time user. But that could also be a personal preference. Thanks for checking this out!

@whitlockjc
Copy link
Member

I get it. I'll have to think of a good way to handle this. I could add a message along the lines of: To see more details, run with the --json flag or something similar. If we had a --verbose flag, we could use it in that message as well. So a new user would be told they could get more information if they wanted but for people wanting a clean output, they get it by default.

As for --json vs. --verbose, I think --json is pretty good as-is but we could do more with --verbose. We could stick with the pretty printing as we have now but show more of the information. (For data that is not a primitive, we could show the first level of values to avoid cluttering up the CLI.)

What do you think?

@avishaan
Copy link
Author

I think that's a good idea. Allows the new user to see how to quickly get more information on their error with minimal impact to someone who doesn't want that information by default.

The only consideration about --verbose vs --json is the first thing I intuitively try is --verbose when trying to get more detail about how to fix my error across other command line tools.

@whitlockjc
Copy link
Member

Good to know. I'm going to close this merge request and instead create a new issue for adding the --verbose flag. Let me know if you're okay with this and if so, we'll go from there.

@theganyo
Copy link
Contributor

It could just be an alias where you could use either --json or --verbose.

@avishaan
Copy link
Author

👍

@whitlockjc
Copy link
Member

Done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants