Skip to content
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

Modifies ExamplesTraitValidator to handle cases where both output and error are defined #1599

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

jvschneid
Copy link
Contributor

@jvschneid jvschneid commented Feb 1, 2023

Issue #, if available:
#1582

Description of changes:
This change modifies the behavior of the ExamplesTraitValidator when
both output and error are defined. This is achieved by treating the
output member as optional, only validating it if it's present and
failing when it's defined alongside the error member.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This commit changes the behavior of the ExamplesTraitValidator when
both output and error are defined. This is achieved by treating the
output member as optional, only validating it if it's present and
failing when it's defined alongside the error member.
boolean isOutputDefined = example.getOutput().isPresent();
boolean isErrorDefined = example.getError().isPresent();

if (isOutputDefined && isErrorDefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the behaviour we want, this needs to be mentioned in the doc as well.
https://smithy.io/2.0/spec/documentation-traits.html?highlight=example#examples-trait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point!

@jvschneid jvschneid marked this pull request as ready for review February 3, 2023 13:54
@jvschneid jvschneid requested a review from a team as a code owner February 3, 2023 13:54
Comment on lines 63 to 64
} else {
if (isErrorDefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the if statements for isErrorDefined and isOutputDefined can be moved into the outer layer as else if statements.

@jvschneid jvschneid marked this pull request as draft February 3, 2023 18:51
@jvschneid jvschneid marked this pull request as ready for review February 8, 2023 16:22
@@ -133,8 +133,8 @@ public ObjectNode getInput() {
/**
* @return Gets the output object.
*/
public ObjectNode getOutput() {
return output;
public Optional<ObjectNode> getOutput() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of a breaking change... but maybe using this trait in code is fringe enough to not matter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about keeping it as is and checking if example.getOutput().isEmpty() to check if it was defined, but it opened the possibility of the example below being valid, which I think shouldn't be, if it's present it should conform, even if empty:

$version: "2.0"

namespace example.hello

@examples([
    {
        title: "Unhappy path",
        output: {},
        error: {
            shapeId: YouShallNotPass
            content: {
                "message": "Go back to the Shadow"
            }
        }
    }
])
operation Hello {
    output: Greeting,
    errors: [YouShallNotPass]
}

structure Greeting {
    @required
    message: String
}

@error("client")
structure YouShallNotPass {
    @required
    message: String
}

@jvschneid jvschneid merged commit 43b13da into smithy-lang:main Feb 23, 2023
@jvschneid jvschneid deleted the examples-trait-validator-fix branch February 23, 2023 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants