Skip to content

Conversation

jareyesda
Copy link
Contributor

@jareyesda jareyesda commented Aug 8, 2021

Lambda.Context changed from a class to a struct.

Motivation:

As described in issue #215. Lambda.Context is currently a class that is a wrapper around values. Because it should be a value type, it was changed from a class to a struct.

Modifications:

The modifications are nearly identical to the changes from #167 - minus the addition of Baggage. The change from a class to a struct also follows CoW semantics to keep performance high. Line 104 of Sources/AWSLambdaRuntimeCore/LambdaRunner.swift had "convenience" and "file private" removed as well.

There are two differences between #167 and this modification: 1) At the time of forking, Tests/AWSLambdaRuntimeTests/Lambda+CodeableTest.swift was not present. 2) There is no Baggage

Result:

Lambda.Context is now a CoW-boxed struct instead of a class.

Note:

This is my first contribution. Please advise on how I can improve future contributions and the proper methodology on testing this change. Thank you all in advance :)

@swift-server-bot
Copy link

Can one of the admins verify this patch?

6 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@fabianfett fabianfett requested review from tomerd and fabianfett August 9, 2021 07:41
@fabianfett fabianfett added the ⚠️ semver/major Breaks existing public API. label Aug 9, 2021
@fabianfett
Copy link
Member

@swift-server-bot test this please

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

@jareyesda Thanks for looking into this. From my side everything looks great so far. Let's see what CI and @tomerd think.

@fabianfett
Copy link
Member

Cool CI is green except for code-formatting. You will need to install swiftformat in version 0.47.3 on your mac. And run swiftformat in your lambda folder. If you need help achieving this, reach out :)

@ktoso
Copy link
Contributor

ktoso commented Aug 9, 2021

This seems based on my previous work — I’m out today, can you give me a day tomorrow to double check this? Thanks

@ktoso ktoso self-requested a review August 9, 2021 11:18
@fabianfett fabianfett added this to the 1.0.0-alpha.1 milestone Aug 9, 2021
@jareyesda
Copy link
Contributor Author

Cool CI is green except for code-formatting. You will need to install swiftformat in version 0.47.3 on your mac. And run swiftformat in your lambda folder. If you need help achieving this, reach out :)

Awesome. I'll go ahead and implement it. I'll follow up if I have any questions!

@jareyesda
Copy link
Contributor Author

This seems based on my previous work — I’m out today, can you give me a day tomorrow to double check this? Thanks

Please do! I would love some feedback

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Okey, this seems good AFAICS.
Thanks for giving me a moment to have a look.

@ktoso
Copy link
Contributor

ktoso commented Aug 10, 2021

Still the formatting issue pending though it seems?

@jareyesda
Copy link
Contributor Author

Still the formatting issue pending though it seems?

Yes. Once I get home I will fix the formatting issue

@fabianfett
Copy link
Member

@swift-server-bot test this please

@fabianfett fabianfett merged commit 774bbf1 into swift-server:main Aug 11, 2021
stevapple added a commit to stevapple/swift-tencent-scf-runtime that referenced this pull request Sep 24, 2021
stevapple added a commit to stevapple/swift-tencent-scf-runtime that referenced this pull request Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ semver/major Breaks existing public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants