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

Add S3BatchEvent and S3BatchResult #231

Merged
merged 4 commits into from
Jun 23, 2022

Conversation

skirsdeda
Copy link
Contributor

S3 batch ops event/result types. Cheers!

@armanbilge
Copy link
Member

Thanks for the contribution! Looks like this needs sbt headerCreateAll, sorry 😅

@skirsdeda
Copy link
Contributor Author

Doesn't like 2022 copyright year, I guess 🙁

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few small nits, otherwise looking good!

implicit def kernelSource: KernelSource[S3BatchEvent] = KernelSource.emptyKernelSource
}

final case class S3BatchJob(id: String)
Copy link
Member

Choose a reason for hiding this comment

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

(r.invocationSchemaVersion, r.treatMissingKeysAs, r.invocationId, r.results))
}

sealed trait S3BatchResultCode
Copy link
Member

Choose a reason for hiding this comment

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

Admittedly, it's kind of a crazy name 😂

Suggested change
sealed trait S3BatchResultCode
sealed abstract class S3BatchResultResultCode

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d18a30127146217038c61060b06e96713daa706c/types/aws-lambda/trigger/s3-batch.d.ts#L35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Any reason why abstract class works better than trait here?

Copy link
Member

@armanbilge armanbilge Jun 23, 2022

Choose a reason for hiding this comment

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

Good question, in practice it will probably make no measurable difference. I prefer abstract class where possible because admittedly, superstition, 😂 I think it might be more performant to do isInstanceOf checks and stuff.

More interestingly, the new enum keyword in Scala 3 would let us define it like this as:

enum S3BatchResultResultCode:
  case Succeeded, TemporaryFailure, PermanentFailure

And behind-the-scenes, that is generating code equivalent to using an abstract class. So this approach is binary compatible with that syntax sugar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, makes sense.

Comment on lines +44 to +45
type S3BatchLambdaEnv[F[_]] = LambdaEnv[F, S3BatchEvent]
type SnsLambdaEnv[F[_]] = LambdaEnv[F, SnsEvent]
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this missing one! :)

@armanbilge armanbilge merged commit 1e21b07 into typelevel:main Jun 23, 2022
@ecastilla95 ecastilla95 mentioned this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants