Add yaml support #227

jayvdb · 2022-11-07T00:43:54Z

No description provided.

jclulow · 2022-11-07T02:17:58Z

I think it would be better to make the format choice explicit using a flag of some kind (e.g., -f, --format <FORMAT>) rather than trying to guess by looking in the file, so that you would then invoke:

$ progenitor -f yaml -i your/openapi/document ...

The default value for -f would be json if not specified, for compatibility.

ahl · 2022-11-08T19:13:21Z

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?

jclulow · 2022-11-08T19:40:10Z

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.

jayvdb · 2022-11-08T22:23:00Z

Note other readers often use some sort of trick to automate this, either file extension, looking for {, and HTTP content-type.
e.g. https://github.com/paperclip-rs/paperclip/blob/43cfea1/src/v2/mod.rs#L110
https://github.com/kstasik/schema-tools/blob/aadd4b5/src/schema.rs#L70
https://github.com/maxdeviant/sdkgen/blob/58db7b41/src/sdkgen/src/main.rs#L56

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:

an explicit CLI arg,
content-type (would exist at this level when Support external $ref #228 is implemented)
file extension,
detect using {

i.e. the CLI arg would not default to json, but if supplied it overrides all.

ahl · 2022-11-20T02:06:42Z

I think it is better to be strict.

I think it it sometimes better to be string and sometimes better to be relaxed. Admittedly this is a harder position to navigate.

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.

What negative consequence would you imagine happening?

ahl

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.

ahl · 2023-01-05T00:55:24Z

progenitor/src/main.rs

+    let mut f = File::open(p)?;
+
+    let mut buf = [b' '];
+    while buf[0].is_ascii_whitespace() {


this doesn't seem right: a json file could start with whitespace, couldn't it?

This loops ignores all leading whitespace in the file, so that the if below is checking whether the first non-whitespace character is {.

right! fair enough. Sounds like you prefer this to the options I mentioned?

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.

yes, agreed. I suggested that we try to parse as json and then, failing that, parse as yaml. Would that not suffice?

Yea, I like that, and will do that next time I revisit this PR - should be later this week.

ahl

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

ahl · 2023-01-12T00:27:52Z

progenitor-impl/tests/test_output.rs

+    if openapi_file.contains('.') {
+        in_path.push(openapi_file);
+    } else {
+        in_path.push(format!("{}.json", openapi_file));
+    }


just add .json to the callers

ahl · 2023-01-12T00:29:20Z

progenitor-macro/src/lib.rs

+/// The `spec` key is required; it is the OpenAPI document path from which the
+/// client is derived, and may be either JSON or YAML.


Suggested change

/// 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.

ahl · 2023-01-12T00:33:16Z

progenitor/src/lib.rs

@@ -11,6 +11,7 @@
 //! README](https://github.com/oxidecomputer/progenitor/README.md)

 pub use progenitor_client;
+pub use progenitor_impl::util::load_api;


I don't think this should be in the progenitor public API

ahl · 2023-01-12T00:37:42Z

progenitor-impl/src/lib.rs

+    #[error("invalid file location {0}")]
+    InvalidFileLocation(String),
+    #[error("unrecognised file contents {0}")]
+    InvalidFileContents(String),


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!

yup, ok 👍

jayvdb force-pushed the support-yaml branch from d0f876b to 250ee73 Compare January 3, 2023 02:33

ahl requested changes Jan 5, 2023

View reviewed changes

jayvdb force-pushed the support-yaml branch from 250ee73 to eb38446 Compare January 12, 2023 00:22

ahl requested changes Jan 12, 2023

View reviewed changes

Add yaml support

3fa339e

jayvdb force-pushed the support-yaml branch from eb38446 to 3fa339e Compare January 12, 2023 03:44

ahl approved these changes Jan 14, 2023

View reviewed changes

ahl merged commit 1ef131a into oxidecomputer:main Jan 14, 2023

jayvdb mentioned this pull request Mar 17, 2023

Add yaml blst-security/cherrybomb#117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add yaml support #227

Add yaml support #227

jayvdb commented Nov 7, 2022

jclulow commented Nov 7, 2022 •

edited

Loading

ahl commented Nov 8, 2022

jclulow commented Nov 8, 2022 •

edited

Loading

jayvdb commented Nov 8, 2022

ahl commented Nov 20, 2022

ahl left a comment

ahl Jan 5, 2023

jayvdb Jan 5, 2023

ahl Jan 5, 2023

jayvdb Jan 5, 2023

ahl Jan 5, 2023

jayvdb Jan 8, 2023 •

edited

Loading

ahl left a comment

ahl Jan 12, 2023

ahl Jan 12, 2023

ahl Jan 12, 2023

ahl Jan 12, 2023

jayvdb Jan 12, 2023

		/// The `spec` key is required; it is the OpenAPI document path from which the
		/// client is derived, and may be either JSON or YAML.

Add yaml support #227

Add yaml support #227

Conversation

jayvdb commented Nov 7, 2022

jclulow commented Nov 7, 2022 • edited Loading

ahl commented Nov 8, 2022

jclulow commented Nov 8, 2022 • edited Loading

jayvdb commented Nov 8, 2022

ahl commented Nov 20, 2022

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb Jan 8, 2023 • edited Loading

Choose a reason for hiding this comment

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jclulow commented Nov 7, 2022 •

edited

Loading

jclulow commented Nov 8, 2022 •

edited

Loading

jayvdb Jan 8, 2023 •

edited

Loading