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

Ensure that the event listener gets all the events #1698

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

sugmanue
Copy link
Contributor

@sugmanue sugmanue commented Mar 30, 2023

Description of changes:

Currently not all events are passed to the listener given the way this is implemented, e.g., if some part of logic add a set of events directly to the list using addAll. In particularLoaderShapeMap adds all the events to the events list for each ShapeModifier here which are then not shown in the output. Validation for the following model

$version: "2.0"

namespace com.amazon.example
structure Mixin1 {
    a: String
}

structure Valid with [Mixin1] {
    a: Integer
}

Will currently output

$ smithy validate ~/tmp/test-model5.smithy
FAILURE: Validated 211 shapes (ERROR: 1)

After this change it will output

ERROR: com.amazon.example#Valid$a (Model)
     @ /Users/sugmanue/tmp/test-model5.smithy
     |
  21 |     a: Integer
     |     ^
     = Member conflicts with an inherited mixin member: com.amazon.example#Mixin1$a

FAILURE: Validated 211 shapes (ERROR: 1)

This example shows that we should probably create an abstraction to contains the ValidationEvents instead of just using a raw List.

EDIT

After the recent change made by Michael this error show up in the output since now the validation events are processed in a different way, however we're still not sending those to the listener, so I think is still worth patching.

@sugmanue sugmanue requested a review from a team as a code owner March 30, 2023 20:49
Copy link
Member

@mtdowling mtdowling left a comment

Choose a reason for hiding this comment

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

Fix extra line then ship

public boolean addAll(Collection<? extends ValidationEvent> validationEvents) {
for (ValidationEvent e : validationEvents) {
validationEventListener.accept(e);

Copy link
Member

Choose a reason for hiding this comment

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

nit - extra line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and merged, thanks.

@sugmanue sugmanue merged commit 56212b7 into smithy-lang:main Mar 31, 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.

2 participants