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

Extend validation event ids to make them unique #1527

Merged
merged 7 commits into from
Feb 15, 2023

Conversation

rchache
Copy link
Contributor

@rchache rchache commented Dec 7, 2022

Description of changes:
We have a need to record and store "approvals" on a per-smithy-violation basis. The idea is that once approved, the same violation will be hidden as long as an "approval" can be matched to it. However, we realized an issue where ValidationEvents do not currently contain the granularity we need. Currently, some linters/validators raise multiple violations on the same shape, and the only difference is the message/source location (Enum casing validation or unstable trait validation for ex). However if we key or hash on the message/source location, we risk being broken if the message or source location changes slightly.

To accomplish this goal, we can use hierarchical eventIds to pack distinguishing info into the eventId.

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

@rchache rchache requested a review from a team as a code owner December 7, 2022 20:19
@mtdowling
Copy link
Member

mtdowling commented Dec 7, 2022

Thanks! We'll go through and review each of these. I do think though that the JMESPath related IDs are too granular and won't be that useful considering it's fine to change JMESPath expressions in waiters as APIs evolve, bugs are fixed, or they're simplified. Let's look out for other cases like that too in this PR.

Edit: Oh also, event are typically already specific to shape IDs, so I don't think we need to add shape IDs to everything. When trying to identify unique events, it should be a combination of shape ID + event ID. So let's make sure to not add shape IDs to events that are already specific to shapes since it doesn't add any value and will just make the event more complex than it needs to be.

@mtdowling mtdowling self-assigned this Dec 19, 2022
@rchache
Copy link
Contributor Author

rchache commented Dec 23, 2022

Addressed all comments except the JMES one which does not yet have a clear path forward

@rchache rchache requested review from sugmanue and mtdowling and removed request for sugmanue January 31, 2023 22:41
@sugmanue sugmanue merged commit 2eef778 into smithy-lang:main Feb 15, 2023
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.

3 participants