chore(iot-events): support all binary operators#19889
Conversation
|
|
| /** | ||
| * Create a expression as String from the given string. | ||
| */ | ||
| public static asString(value: string): Expression { |
There was a problem hiding this comment.
Why the rename from fromString to asString ? asString implies a conversion to me, which is not what is happening.
There was a problem hiding this comment.
Currently, when users want to use literal string in the expression, it is needed to use fromString with double-quotes as following:
fromString('"my-data"')And when users want to use literal number, it is needed to use fromString with string as following:
fromString('12.5')Actually, I want to provide fromString and fromNumber as following:
fromString('my-data') // double-quotes is not needed
fromNumber(12.5) // apply number instead of stringBut already I have used fromString name space...
literalString and literalNumber are better?
There was a problem hiding this comment.
In this case, the iotevents-actions package seems to be experimental, which means we are technically allowed to break its behavior in breaking ways if we figure out we made a mistake in the past (that's what the point of that stability is).
So I'd prefer for us to keep the prime real estate names, and change the behavior of fromString and fromNumber to have the desired behavior.
If you are concerned about inconveniencing users, fair enough and I love that instinct. We can choose to do one of the following things:
- Introduce a feature flag to gate the old vs new behavior of the
fromString()method. - Introduce a new method (immediately
@deprecated) namedlegacyFromString()or something, and clearly explain in the PR body that users relying on the old behavior should either fix their code or switch to the other method.
There was a problem hiding this comment.
@rix0rrr
Thanks for your suggestion!
I have reverted the commit for asString and asNumber. And I will implement fromString and fromNumber as breaking change in another PR. Because it will have small changing some tests already written.
There was a problem hiding this comment.
I'm creating the PR yamatatsu#30 for breaking change of the behavior of Expression.fromString().
|
@yamatatsu Ouch. Another one our branch change blew up. Sorry that the lag in our reviews has caused so much extra work overall. |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Just putting this in changes requested so that I'll be able to tell more easily when it's been rebased.
a63fc40 to
bcda92d
Compare
Pull request has been modified.
|
@TheRealAmazonKendra |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This PR adds rest binary operators of IoT Events.
This PR does not add new integ tests because of this conversation.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license