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

Use zio.Config.Switch for discriminated sum types #1167

Merged
merged 9 commits into from
May 15, 2023

Conversation

guersam
Copy link
Contributor

@guersam guersam commented May 7, 2023

@guersam guersam marked this pull request as ready for review May 7, 2023 14:30
@guersam guersam requested a review from a team as a code owner May 7, 2023 14:30
@guersam
Copy link
Contributor Author

guersam commented May 7, 2023

Fixed Scala 3 derivation, ignoring failing MarkdownSpec for now.

ConfigDocs.DynamicMap(
loop(descriptions, c, latestPath, alreadySeen),
map.map { case (k, v) =>
k.toString -> loop(descriptions, v, latestPath, alreadySeen)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In most cases k will be String, but I wonder if there could be a better support for polymorphic key type of Config.Switch[A, B].

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's going to be hard at DynamicMap level in zio core, as this key gets propagated all over.

@@ -9,7 +9,7 @@ import java.time.{LocalDate, LocalDateTime, LocalTime}
import java.util.UUID
import scala.collection.immutable

final case class DeriveConfig[T](desc: Config[T], isObject: Boolean = false) {
final case class DeriveConfig[T](desc: Config[T], isObject: Boolean = false, constValue: Option[T] = None) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary to support Config.Switch, but I wonder if there is a better way, or at least replace isObject with constValue.isDefined.


object Metadata {
final case class Object(name: ProductName) extends Metadata
final case class Object[T](name: ProductName, constValue: T) extends Metadata
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this T look okay?

@@ -210,7 +210,7 @@ object MarkdownSpec extends BaseSpec {

assert(result)(equalTo(expectedMarkdown))
}
)
) @@ TestAspect.ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, what's the purpose of this spec which exists only in Scala 3?

sealed trait Credentials

case class Password(value: String) extends Credentials

case class Token(value: String) extends Credentials

case object InstanceProfile extends Credentials
Copy link
Contributor Author

@guersam guersam May 9, 2023

Choose a reason for hiding this comment

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

In addition to preventing match errors, this PR enables this specific use case - discriminated sealed traits having case objects as children.

case config: FallbackWith[B] => FallbackWith(loop(config.first), loop(config.second), config.f)
case config: Fallback[B] => Fallback(loop(config.first), loop(config.second))
case Sequence(config) => Sequence(loop(config))
case Switch(config, map) => Switch(config, map.map { case (k, v) => k -> loop(v) })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, I wonder if Config#mapKey extension method is still necessary, because now I found it more idiomatic to adjust the keys using ConfigProvider. (e.g. ConfigProvider#kebabCase)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we will keep it for now.

@@ -343,8 +344,7 @@ trait ConfigSyntax {
// override def flatten: IndexedFlat = indexedFlat
// }


//Disabled until next version of ZIO: https://github.com/zio/zio-config/blob/avoid_custom_index_until_3/README.md#indexed-map-array-datatype-and-a-some-implementation-notes
// Disabled until next version of ZIO: https://github.com/zio/zio-config/blob/avoid_custom_index_until_3/README.md#indexed-map-array-datatype-and-a-some-implementation-notes

Choose a reason for hiding this comment

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

Looks like the change was unrelated, but this link appears to be dead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the whole function is obsolete as ConfigProvider.fromMap is builtin now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afsalthaj
Copy link
Collaborator

Could you resolve conflicts? May be that will fix the format changes, so we could focus only the specific changes.

@guersam
Copy link
Contributor Author

guersam commented May 14, 2023

@afsalthaj Resolved conflicts.

@@ -26,13 +26,17 @@ object AutomaticConfigTest extends ZIOSpecDefault {
)
}

object AutomaticConfigTestUtils {
object AutomaticConfigSpecUtils {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For API consistency, I'd like to make this spec shared between Scala 2 and 3. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, something that was in my radar as well. Agree.

* as `keyName`.
*
* Example:
* {{{
* @nameWithLabel("type")
* @discriminator("type")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice 👍

@afsalthaj
Copy link
Collaborator

Thanks for the contribution.

@afsalthaj afsalthaj merged commit 07c878e into zio:series/4.x May 15, 2023
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.

3 participants