Skip to content
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(core): Size.bytes() #24136

Merged
merged 4 commits into from
Feb 15, 2023
Merged

feat(core): Size.bytes() #24136

merged 4 commits into from
Feb 15, 2023

Conversation

pattasai
Copy link
Contributor

@pattasai pattasai commented Feb 13, 2023

This PR adds Size.bytes(), which allows to specify a Size class from an amount of bytes. Within the Size class, Size.bytes( ) is the additional method added here to enable support for bytes as well as conversion to bytes with Size.toBytes.

For example,

const size = new Size.bytes(1024);
expect(size.toKibibytes).toEqual(1);

creates a new object of the Size class with a size of 1024 bytes and which is equivalent to 1 kibibyte.

Closes #24106.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the p2 label Feb 13, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team February 13, 2023 15:24
@gitpod-io
Copy link

gitpod-io bot commented Feb 13, 2023

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 13, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

Comment on lines 265 to 275
/**
* A nullable integer that is used to enable compression (with non-negative
* between 0 and 10485760 (10M) bytes, inclusive) or disable compression
* (when undefined) on an API. When compression is enabled, compression or
* decompression is not applied on the payload if the payload size is
* smaller than this value. Setting it to zero allows compression for any
* payload size.
*
* @default - Compression is disabled.
*/
readonly minimumCompressionSize?: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! This change wasn't supposed to be in this PR :)

@@ -648,6 +660,7 @@ export class SpecRestApi extends RestApiBase {
name: this.restApiName,
policy: props.policy,
failOnWarnings: props.failOnWarnings,
minimumCompressionSize: props.minimumCompressionSize,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nor this one.

Comment on lines 1029 to 1052

test('SpecRestApi minimumCompressionSize test', () => {
// GIVEN
const app = new App({
context: {
'@aws-cdk/aws-apigateway:disableCloudWatchRole': true,
},
});

const stack = new Stack(app);
const api = new apigw.SpecRestApi(stack, 'SpecRestApi', {
apiDefinition: apigw.ApiDefinition.fromInline({ foo: 'bar' }),
minimumCompressionSize: 10485760,
});

// WHEN
api.root.addMethod('GET');

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::ApiGateway::RestApi', {
Name: 'SpecRestApi',
MinimumCompressionSize: 10485760,
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nor this one.

Comment on lines 203 to 207
public static readonly Kibibytes = new StorageUnit('kibibytes', 1);
public static readonly Bytes = new StorageUnit('bytes', 1 / 1024);
public static readonly Mebibytes = new StorageUnit('mebibytes', 1024);
public static readonly Gibibytes = new StorageUnit('gibibytes', 1024 * 1024);
public static readonly Tebibytes = new StorageUnit('tebibytes', 1024 * 1024 * 1024);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 1 / 1024 makes me uneasy. Computers aren't good at representing fractional numbers and getting math with them correct.

Can we shift all fractions up by 1024 ? Make bytes = 1, kibibytes = 1024, etc ?

const size = Size.bytes(1_073_741_823);
expect(size.toBytes()).toEqual(1_073_741_823);

});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple more tests here to be sure. Especially conversions between bytes and other units will be interesting.

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 and removed p2 labels Feb 13, 2023
@kaizencc kaizencc added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Feb 14, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review February 14, 2023 14:32

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the formatting of the PR body:

image

And try to write your PR body as a couple of sentences that introduce the motivation for the change and the changes that were made.

Also please still get rid of the changes that weren't supposed to be in this PR.

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 15, 2023

If you don't know what to write, in the PR body, take a look at some other PRs to get inspiration. I will link a couple of mine here, so you can see what I'm looking for:

#24175
#24053
#24027
#23986

@rix0rrr rix0rrr changed the title feat(core): support setting size class in bytes feat(core): Size.bytes() Feb 15, 2023
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Feb 15, 2023
@kaizencc kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Feb 15, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 15, 2023

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 0ea40c6
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 9b2a45a into main Feb 15, 2023
@mergify mergify bot deleted the pattasai-Sizeclass branch February 15, 2023 20:36
@mergify
Copy link
Contributor

mergify bot commented Feb 15, 2023

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).

@pattasai pattasai self-assigned this Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core: Size class does not support bytes
4 participants