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

Remove reflected values from validation message #695

Merged
merged 4 commits into from
Feb 27, 2023

Conversation

srchase
Copy link
Contributor

@srchase srchase commented Feb 16, 2023

This PR updates the generateValidationMessage to not return the input value in the validation message.

This change will be paired with updated protocol tests: smithy-lang/smithy#1622

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@srchase srchase requested review from a team as code owners February 16, 2023 18:52
@@ -170,5 +153,5 @@ export const generateValidationMessage = (failure: ValidationFailure): string =>
suffix = "must have unique values";
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we are missing tests for this case. can you add some?

Copy link
Contributor Author

@srchase srchase Feb 18, 2023

Choose a reason for hiding this comment

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

Added here, and to the protocol tests in smithy-lang/smithy#1622.

@srchase srchase marked this pull request as draft February 16, 2023 19:35
@srchase srchase marked this pull request as ready for review February 18, 2023 00:38
@cmoher cmoher requested a review from gosar February 22, 2023 16:10
path: "/test",
};
expect(generateValidationMessage(failure)).toEqual(
"Value with repeated values at '/test' failed to satisfy constraint: Member must have unique values"
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads odd. Was ok when it was Value [80, 80] with repeated values at '/test'.... But now, probably better to say Value at 'test'... similar to the other messages. Would have to update the protocol tests too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok to merge this now and update when the protocol test is updated.

@gosar gosar self-requested a review February 27, 2023 19:22
Copy link
Contributor

@gosar gosar left a comment

Choose a reason for hiding this comment

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

Ok to merge this now and update when the protocol test is updated.

@srchase srchase merged commit 9eb56cc into smithy-lang:main Feb 27, 2023
@srchase srchase deleted the validation-value-reflection branch February 27, 2023 22:45
srchase added a commit to srchase/smithy-typescript that referenced this pull request Mar 17, 2023
* Remove reflected values from validation message
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