-
Notifications
You must be signed in to change notification settings - Fork 217
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
Add configurable diff evaluator #2056
base: main
Are you sure you want to change the base?
Add configurable diff evaluator #2056
Conversation
Adds a smithy-diff evaluator which can be configured via metadata property `diffEvaluators` within the model. Unlike the similar EmitEach/EmitNoneSelectorValidators, this diff evaluator is not loaded using SPI, and the ability to create your own custom configurable diff evaluators has not been exposed. Instead, smithy-diff just looks at the new model's metadata and loads any diff evaluators directly. We can expose creating custom configurable diff evaluators later if there is a need, but this diff evaluator should be enough for almost every use case, and we can also extend it easily with more configuration options. This evaluator works as follows: 1. Get a subset of shapes to match based on the `appliesTo` property. Currently either added shapes or removed shapes. 2. Optionally filter this subset of shapes further with a selector configured in the `filter` property, which runs on either the new or old model depending on `appliesTo`. 3. Run the configured `selector` on either the new model or old model depending on `appliesTo`. 4. Match shapes returned by `selector` to the set of shapes from 2, and emit events based on `emitCondition`. This functionality can be extended later by adding more options for `emitCondition` and `appliesTo`. For example, there is currently no configuration option for looking at changed shapes, but you could imaging wanting to see which shapes don't match in the old model, but do in the new model, which would be a new `appliesTo`. Another `emitCondition` could be `IfAnyDontMatch` or something. Also adds a validator to smithy-diff for this metadata property so you'll know when the model is being build if evaluators have been configured incorrectly. This requires having a dependency on smithy-diff in the model package where you're configuring the diff evaluators, but the alternative is stick the validation in smithy-model, creating an implicit dependency on smithy-diff.
273a14c
to
5c8f6a9
Compare
selector: "[trait|default = 0]" | ||
} | ||
{ | ||
id: "AddedMemberWithoutClientOptional" |
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.
Can you provide an example of IfAnyMatch that is more realistic?
id: "RemovedRootShape" | ||
message: "Removed root shape." | ||
emitCondition: "ForEachMatch" | ||
appliesTo: "RemovedShapes" |
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.
Can we get more removed examples?
{ | ||
id: "AddedMemberWithoutClientOptional" | ||
message: "One of the added members does not have `@clientOptional` trait." | ||
emitCondition: "IfAnyMatch" |
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.
Is there any use case for IfNoneMatch?
@@ -0,0 +1,54 @@ | |||
$version: "2.0" | |||
|
|||
metadata diffEvaluators = [ |
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.
Maybe this should emulate validators: https://smithy.io/2.0/spec/model-validation.html#validators. So more like:
{
id: "AddedInternalOperation"
name: "EmitEachMatch|EmitIfNoneMatch|EmitIfAnyMatch|..."
severity: "NOTE"
configuration: {
appliesTo: "AddedShapes|RemovedShapes|ChangedShapes"
selector: "operation [trait|internal]"
}
}
This keeps the door open for future configurable diff evaluators, and maybe even the ability to configure built-in evaluators. This could also, if we really look into the future, potentially cover metadata diffs and trait diffs too.
Adds a smithy-diff evaluator which can be configured via metadata property
diffEvaluators
within the model. Unlike the similar EmitEach/EmitNoneSelectorValidators, this diff evaluator is not loaded using SPI, and the ability to create your own custom configurable diff evaluators has not been exposed. Instead, smithy-diff just looks at the new model's metadata and loads any diff evaluators directly. We can expose creating custom configurable diff evaluators later if there is a need, but this diff evaluator should be enough for almost every use case, and we can also extend it easily with more configuration options.This evaluator works as follows:
appliesTo
property. Currently either added shapes or removed shapes.filter
property, which runs on either the new or old model depending onappliesTo
.selector
on either the new model or old model depending onappliesTo
.selector
to the set of shapes from 2, and emit events based onemitCondition
.This functionality can be extended later by adding more options for
emitCondition
andappliesTo
. For example, there is currently no configuration option for looking at changed shapes, but you could imaging wanting to see which shapes don't match in the old model, but do in the new model, which would be a newappliesTo
. AnotheremitCondition
could beIfAnyDontMatch
or something.Also adds a validator to smithy-diff for this metadata property so you'll know when the model is being build if evaluators have been configured incorrectly. This requires having a dependency on smithy-diff in the model package where you're configuring the diff evaluators, but the alternative is stick the validation in smithy-model, creating an implicit dependency on smithy-diff.
See the added test case for an example.
Next Steps:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.