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): add DetectorModel L2 Construct #18049
feat(iotevents): add DetectorModel L2 Construct #18049
Changes from 6 commits
e5f18b2
7873d17
07081ed
3d2db07
9c24a19
0723a1c
8f82cbb
fceea45
8ae95cd
ff69d77
d197256
f3511b2
e5baa25
581ddef
50c183c
971d5c4
fbfe028
0b6600d
18e86d0
9744d78
d89d0a0
6ff671e
78fc88c
5506cb5
087bb42
2b4301e
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.
This function is only used in the
state.ts
file, right?Can we move it there then, and make it non-exported?
In general, we should try to minimize the surface area of the exported library API as much as possible, and I don't think there's a reason to surface this function to users of this module.
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.
Right, I'll move it!
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 find the constant name changes here super confusing 😕. This is a good example. We are writing documentation for a property called
onEnterEvents
, its type isEvent[]
, and the description says "Specifies the actions that are performed". Doesn't even mention the word "event" anywhere.Are we missing something here? Should what in this PR is called an
Event
actually be some sort of Action?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.
Naming
In CloudFormation,
State
hasOnEnter
andOnEnter
has onlyevents
(typed asEvent[]
). I easily concatedonEnter
andevents
...If I named it just
onEnter
, would it be less uncomfortable?JSDoc
I should fix it!
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 fix it in
78fc88c
(#18049).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.
Can we make this method
@internal
? I don't think there's a need to expose it in the public API of this module.It will require renaming it to
_toStateJson()
.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 this method. This is actually a
DetectorModel
concern, not aState
concern, that this validation has to be performed.Can we remove this method, and perform this validation in
DetectorModel
by callingtoStateJson()
on itsinitialState
instead?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.
If we implement this validation with
toStateJson()
, It will be as following I think:But we can't get
onEnter?.events
becauseonEnter
can becdk.IResolvable
, same forevents.some()
andevent.condition
.Should
toStateJson()
return more exact type that is compatible withCfnDetectorModel.StateProperty
instead ofCfnDetectorModel.StateProperty
itself?Following is the type
CfnDetectorModel.StateProperty
thattoStateJson()
return: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, I see the problem. I think we could just check whether
onEnter
is aCfnDetectorModel.OnEnterProperty
, and skip the validation if it's anIResolvable
. But I don't want you to write all of that code if a simpler alternative is possible here.So, let's keep
_toStateJson()
as it is now, but let's make this method@internal
, and rename it to_onEnterEventsHaveAtLeastOneCondition()
.