Add yaml support#227
Conversation
|
I think it would be better to make the format choice explicit using a flag of some kind (e.g., The default value for |
|
My understanding is that yaml is a superset of json--why would we require users to specify the file type if we don't have to? |
|
I think it is better to be strict. If what I believe that I have is a JSON file, I don't want it to be accidentally mis-parsed as YAML if it is not valid for some reason. |
|
Note other readers often use some sort of trick to automate this, either file extension, looking for Having an automated approach becomes more important when implementing #228 , as the external files may be using a different format than the initial file/url specified on the CLI. e.g. a YAML file may refer to a JSON file. I suggest the following order for determining the format:
i.e. the CLI arg would not default to json, but if supplied it overrides all. |
I think it it sometimes better to be string and sometimes better to be relaxed. Admittedly this is a harder position to navigate.
What negative consequence would you imagine happening? |
ahl
left a comment
There was a problem hiding this comment.
If we're doing this for the command, we should do this for the macro.
Off the cuff, I think my preferences for inferring the type of the file would be
- the file name
- try it as json, fall back to yaml
I don't love option 1. It's kind of gross, and I have plans for the macro that means that the file name may not be as readily available.
I do like option 2 even though it's inefficient:
- If the file is JSON but there's an error, the serde_yaml error will be basically the same
- serde_json is a lot faster so I don't want to just use serde_yaml for everything even though it would work
- Slowing down the error case for a badly formatted file would be fine.
| let mut f = File::open(p)?; | ||
|
|
||
| let mut buf = [b' ']; | ||
| while buf[0].is_ascii_whitespace() { |
There was a problem hiding this comment.
this doesn't seem right: a json file could start with whitespace, couldn't it?
There was a problem hiding this comment.
This loops ignores all leading whitespace in the file, so that the if below is checking whether the first non-whitespace character is {.
There was a problem hiding this comment.
right! fair enough. Sounds like you prefer this to the options I mentioned?
There was a problem hiding this comment.
Well, IMO we need some auto-detection to handle #228 in order to still do the right thing if the content-type isnt recognised , and often raw json & yaml files have strange content-types provided by the server.
There was a problem hiding this comment.
yes, agreed. I suggested that we try to parse as json and then, failing that, parse as yaml. Would that not suffice?
There was a problem hiding this comment.
Yea, I like that, and will do that next time I revisit this PR - should be later this week.
ahl
left a comment
There was a problem hiding this comment.
I'd rather have some non-keeper yaml file since otherwise it will be annoying to sync up the json and yaml files as folks add test cases to them
| if openapi_file.contains('.') { | ||
| in_path.push(openapi_file); | ||
| } else { | ||
| in_path.push(format!("{}.json", openapi_file)); | ||
| } |
There was a problem hiding this comment.
just add .json to the callers
| /// The `spec` key is required; it is the OpenAPI document path from which the | ||
| /// client is derived, and may be either JSON or YAML. |
There was a problem hiding this comment.
| /// The `spec` key is required; it is the OpenAPI document path from which the | |
| /// client is derived, and may be either JSON or YAML. | |
| /// The `spec` key is required; it is the path of the OpenAPI document (JSON or YAML) from which the client is derived. |
| //! README](https://github.com/oxidecomputer/progenitor/README.md) | ||
|
|
||
| pub use progenitor_client; | ||
| pub use progenitor_impl::util::load_api; |
There was a problem hiding this comment.
I don't think this should be in the progenitor public API
| #[error("invalid file location {0}")] | ||
| InvalidFileLocation(String), | ||
| #[error("unrecognised file contents {0}")] | ||
| InvalidFileContents(String), |
There was a problem hiding this comment.
if load_api isn't public we don't need these error cases. I'd prefer copying the code in both progenitor/src/main.rs and progenitor-macro
At some point, progenitor-macro will switch to something that uses include_str!
No description provided.