feat: bitrate utility function#37244
Conversation
|
Exemption Request - I don't believe there are integration tests for core lib (following existing style for Duration in the lib) |
| } | ||
|
|
||
| private constructor(private readonly bps: number) { | ||
| if (bps < 0) { |
There was a problem hiding this comment.
This validation will throw on CDK tokens (CloudFormation parameters, Fn.ref, etc.) because token-encoded numbers use special representations like -1.8881545897087626e+289 that incorrectly trigger the negative check.
Both Duration and Size guard their constructor validation with Token.isUnresolved(). This needs the same treatment:
private constructor(private readonly bps: number) {
if (!Token.isUnresolved(bps) && bps < 0) {
throw new UnscopedValidationError(`Bitrate amounts cannot be negative. Received: ${bps}`);
}
}
Also note the suggested code already includes the value the customer send, so its clearer, please preserve that.
| * This class provides a type-safe way to specify bitrates with clear units, | ||
| * preventing confusion between bits per second, kilobits per second, etc. | ||
| */ | ||
| export class Bitrate { |
There was a problem hiding this comment.
Please add an isUnresolved() method (both Duration and Size have one) so consumers can check if the value they are working with is a token and if it is unresolved
| * | ||
| * This class provides a type-safe way to specify bitrates with clear units, | ||
| * preventing confusion between bits per second, kilobits per second, etc. | ||
| */ |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
|
||
| An instance of `Bitrate` is initialized through one of its static factory methods: | ||
|
|
||
| ```ts |
There was a problem hiding this comment.
Rosetta is complaining about this examples missing the correct referece. Please also review that
alvazjor
left a comment
There was a problem hiding this comment.
In general, all the new additions (token validation, conversion checks, etc) need to have unit tests. Since this is not a construct per se, we cannot do integ test yet, but then we should make sure unit test provide the highest coverage possible. That needs to be changed too, since we are missing test scenarios
|
|
||
| private constructor(private readonly bps: number) { | ||
| if (!Token.isUnresolved(bps) && bps < 0) { | ||
| throw new UnscopedValidationError('Bitrate cannot be negative'); |
There was a problem hiding this comment.
Please add here in the error message the the value they are sending, so the customers understand what is wrong
| return this.bps; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The conversion methods (this and all the ones below) perform arithmetic on the stored value without checking for tokens. When the underlying value is a token, this produces corrupted results that silently generate invalid CloudFormation templates.
There was a problem hiding this comment.
This comment was not addressed. Validations are required before trying do the conversion
There was a problem hiding this comment.
thanks! I've fixed this one properly now
|
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). |
Merge Queue Status
This pull request spent 41 minutes 59 seconds in the queue, including 30 minutes 40 seconds running CI. Required conditions to merge
|
|
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). |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #.
Reason for this change
Description of changes
Introduction of a Bitrate utility function (similar to Duration) which can be used in AWS Elemental Media Services.
Example of usage:
video_bitrate in https://docs.aws.amazon.com/mediapackage/latest/ug/manifest-filtering.html
maxBitrate in https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_mediaconnect.CfnFlow.SourceProperty.html
Describe any new or updated permissions being added
Description of how you validated changes
Added tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license