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 3 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.
I feel like we need to present an API for these conditions - just writing expressions in strings won't cut it I think.
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.
That's sounds great! 😍
Recently, I knew
mapping-tenplate.ts
inaws-appsync
. I wanna try to design like it! (Or is there any other code I can refer to?)(And I wanna try to design IoT Rules expressions that we discussed this earlier. I couldn't imagine implementing it at all before but.)
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 think that's the closest similar thing we have for something like this in the CDK currently, yes.
Can you link me to the AWS docs for this functionality? I want to make sure I have a good understanding of the language that's permitted here.
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.
Sure! But, as far as I can find, the only document that explains "what a condition is" is the CloudFormation document.
This CFn document is described that
condition
is expression returning boolean. The refferences of Expressions are here.And bellow is examples of
condition
fromDetector model examples
.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()
.