#patch Adding support for SdkBindingData[scala.Option[_]]
TL;DR
Adding explicit support for scala.Option type
Type
- [ ] Bug Fix
- [x] Feature
- [ ] Plugin
Are all requirements met?
- [x] Code completed
- [x] Smoke tested
- [x] Unit tests added
- [x] Code documentation added
- [ ] Any pending items have an associated Issue
Complete description
Added explicit special case for Option type when instantiating a generics literal type. Since support for any case classes in SdkLiteralTypes has been introduced, it's not immediately obvious why this works
case class Example(value: Option[String])
SdkLiteralTypes.of[Example]()
but this doesn't:
SdkLiteralTypes.of[Option[String]]()
The latter case is especially useful for input classes to workflows and SdkTransforms of the form
case class Input(
inputStr: SdkBindingData[String],
optional: SdkBindingData[Option[String]],
...
)
The Option type can't be instantiated directly like a common case class, so it needs to be instantiated differently. I believe the common usage of Option as a field type in scala classes justifies this special handling.
Tracking Issue
NA
Follow-up issue
NA
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line: - Most of the repos have a PR template; if not, fill it out to the best of your knowledge. - Sign off your commits (Reference: DCO Guide).
What do you think about add a specific
offactor for Options.
ofOption(value) => SdkBindingData[Option[_]]and
of(Option(value)) => SdkBindingData[Option[_]]to avoid to need to use the generics
How and why do you mean to avoid generics exactly? You'd still want a SdkBindingData[Option[T]] as a result like you have for collections and maps. To avoid generics entirely would require adding ofStringOption, ofIntegerOption, etc. which may be bloating the code.
What could be useful in my opinion may be ofOption[T](value: T): SdkBindingData[Option[T]] and/or of[T : TypeTag](option: Option[T]): SdkBindingData[Option[T]]. The reason being that this of factory will otherwise only work if it is a defined Option (similar to collections and maps needing to contain at least one value), which seems an odd restriction in this case.
Congrats on merging your first pull request! 🎉