Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(iotevents): support transition events #18768
feat(iotevents): support transition events #18768
Changes from 1 commit
cee19c0
674459c
ab42260
f31211e
94685b6
dbc42d0
514a77a
7f52f71
e520bf7
75ad95b
3f3dff0
aebc589
da02f80
a3c094d
beb3341
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I actually don't like the API of this method right now.
ITransitionEvent
.eventName
should be optional, and we should generate an event name by default by combining the names of the States.Options
by convention in the CDK, so this should beTransitionOptions
.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.
So smart! 🤩 Thank you!
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.
Shouldn't we use a Set here, to check if we haven't see the same state twice?
Also, we're using
stateName
to do the check, but I'm 99% sure we don't do any uniqueness checking on state names, so this is not correct - we should actually compare the State objects.So, to sum up, this should use a
Set<State>
for checking the duplicates, and return a list ofCfnDetectorModel.StateProperty
. Don't do any accumulation of the result through mutating a variable - instead, just return an empty list if the given State was already visited, and that will handle this case.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.
I'm amaging that codes became clean by this and below advices!
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.
I have to say, I don't understand why do we have to call this method recursively? Why does translating all
this.transitionEvents
Events isn't good enough?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.
OK.
IoT Events can construct state dependency graph, for example the following image:
When this case,
firstState
has only one transitionfirstState_to_secondState
and can collect only one state. Therefore, it is necessary to recursively collect states by tracing transitions.