-
Notifications
You must be signed in to change notification settings - Fork 120
If all other options fail, generate synthetic schemas #60
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
Conversation
assert.Equal(t, | ||
testFixtures.Resources["customer"].(map[string]interface{})["id"], | ||
data.(map[string]interface{})["customer"]) | ||
{ |
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.
The diff in this test is a bit of a mess unfortunately — most of this didn't change, but I ended up wrapping each individual case in its own scope (i.e., { ... }
) just to help guarantee that we're not sharing tests between any state between them.
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.
Adding ?w=1
to the Github URL makes the diff hide whitespace-only changes, which makes this much more readable.
02dac8d
to
d971f22
Compare
@@ -227,6 +235,12 @@ func (g *DataGenerator) generateInternal(params *GenerateParams) (interface{}, e | |||
return listData, err | |||
} | |||
|
|||
// Generate a synthethic schema as a last ditch effort | |||
if example == nil && schema.XResourceID == "" { | |||
example = &valueWrapper{value: generateSyntheticFixture(schema, context)} |
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.
Should we log a warning to stderr if we generate a synthetic fixture?
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.
Yeah, I like that idea. I ended up adding this code:
if verbose {
// We list properties here because the schema might not have a
// better name to identify it with.
fmt.Printf("Generated synthetic fixture with properties: %s\n",
stringOrEmpty(propertyNames(schema)))
}
I didn't put it in as an error because if we use this for prereleases, we could plausibly expect this to be a pretty standard path. Also, I'm not sure that we can ever expect to have a good fixture for some of our edge resources like "charge outcome rule".
LGTM |
d971f22
to
9d5b805
Compare
Here we codify a path to generate a synthetic schema for objects that couldn't be loaded from fixtures. This is useful in a couple situations: 1. Where we're trying to expand on a deep object that never lives at the top level of the API (see #46). 2. Where we have a schema that doesn't have have fixtures generated for it yet (e.g., an internal or prerelease one). Fixes #46.
9d5b805
to
25082ad
Compare
Thanks Tim! |
Here we codify a path to generate a synthetic schema for objects that
couldn't be loaded from fixtures. This is useful in a couple situations:
top level of the API (see Empty response when expanding an optional unset attribute #46).
it yet (e.g., an internal or prerelease one).
Fixes #46.
r? @tmaxwell I hope it's alright, but just to get the groundwork laid for this
one, I did some other refactoring that wasn't reviewed to nicen things up. The
main thing that changed as that the
Generate
family of functions now take aGenerateParams
as they're being invoked, which allows us to document thingsmore easily, and also prettifies the function signatures because they don't
need to take so many arguments.