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

Bug 142 #153

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Bug 142 #153

wants to merge 3 commits into from

Conversation

mkrzemien
Copy link
Contributor

@mkrzemien mkrzemien commented May 24, 2020

This proposal is somewhat related to original issue #142. It is described by following test-case:

class A()
class B(val oa: Option[A])

object TestSomeOption {
  val a = new A()
  val oa = Some(new A())
  val b = wire[B]
}

object TestSomeValue {
  val a = new A()
  val b = wire[B]
}

object TestNone {
  val b = wire[B]
}

require(TestSomeOption.b.oa.contains(TestSomeOption.oa.get))
require(TestSomeValue.b.oa.contains(TestSomeValue.a))
require(TestNone.b.oa.isEmpty)
  • When a type to wire is Option[A] and an instance of Option[A] exists, it is wired as normal.
  • Otherwise if an instance a of type A exists, it is wired as Some(a).
  • Otherwise it is wired as None.

Current change is not fully complete. In particular more test-cases would be required. It is more of a proof-of-concept. But I would appreciate any comments.

@adamw
Copy link
Member

adamw commented Jun 4, 2020

Hm, I'm not convinced. I'd be afraid that this might lead to confusing behavior, if a parameter is wired as None because it's missing for some possibly not-obvious reason.

@mkrzemien
Copy link
Contributor Author

mkrzemien commented Jun 4, 2020

What about a separate method e.g. wireOpt? Anyway, thanks for the review :)

@adamw
Copy link
Member

adamw commented Jun 4, 2020

Yes, wireOpt might make more sense - it explicitly states the intent. Though then, I would name it wireOption :)

@mkrzemien
Copy link
Contributor Author

One more detail: Currently the new feature allows for nesting: https://github.com/sijarsu/macwire/blob/bug-142/tests/src/test/resources/test-cases/_t142_nested.success).
Would you like me to keep it that way? Or maybe limit it to just this case: https://github.com/sijarsu/macwire/blob/bug-142/tests/src/test/resources/test-cases/_t142.success

@adamw
Copy link
Member

adamw commented Jun 5, 2020

I think Option[Option[A]] is suspicious, so I'd keep it without nesting

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.

2 participants