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

Simplify API to no longer expose internal Option #106

Merged
merged 2 commits into from
Sep 7, 2015

Conversation

stevenheidel
Copy link
Contributor

Motivation: Clump in general does a very good job of hiding its implementation details. For example, it's not required to know about ClumpFlatMap, ClumpOrElse, etc. to use the API. However, I feel like there's one place where this abstraction leaks: namely that values not returned by sources are internally stored as an Option.

This presents a major problem when working with Clump[Option[T]] because several APIs are overloaded and take Option to mean the underlying implementation Option and not the user facing Option.

Here's some examples of real problems we've had with this:

1: It leads to surprising types.

Clump.value(2): Clump[Int] // a value
Clump.value(List(2)): Clump[List[Int]] // a value with a context
Clump.value(Some(2)): Clump[Int] // a value again???

2: It requires awkward constructions to work with Clump[Option[T]].

// How do you create a Clump containing None?
Clump.value(None) // Wrong
Clump.value(Some(None)) // Correct but awkward

3: It can cause very subtle exceptions that are hard to find.

// An engineer on my team wrote this code:
val foo: Clump[Option[Profile]] = ...

foo.recover {
  // The intent here is to create a defined Clump containing None. However, this actually creates an undefined Clump.
  // This is not caught at compile time, and instead will fail ***at runtime*** even if you've defined a fallback for the inner Option.
  case e: D2ServiceException => None
  // This would be the correct line which, from the compilers perspective, is the same as the previous line
  case e: D2ServiceException => Some(None)
}

Solution: The solution is to remove any reference to the inner Option from the outer API. This includes removing any methods which were overloaded to accept both T and Option[T].

Here's how using the API would change:

Desired Type Before After Notes
Clump[T] Clump.value(None) Clump.empty
Clump[Int] Clump.value(Some(3)) Clump.value(3) Although Clump.value(3) already worked so you were probably using that anyway
Clump[Option[Int]] Clump.value(Some(None)) Clump.value(None)
Clump[Int] Clump.future(Future(Some(3))) Clump.future(Future(3)) Although again Clump.future(Future(3)) already worked
Clump[Option[Int]] Clump.future(Future(Some(None))) Clump.future(Future(None))
Clump[T] Clump.fallback(None) Clump.fallbackTo(Clump.empty) fallback becomes fallbackTo
Clump[Int] Clump.fallback(Some(3)) Clump.fallback(3)
Clump[T] Clump.handle { case _ => None } Clump.rescue { case _ => Clump.empty } If one of your cases had None, then handle has to become rescue
Clump[Int] Clump.handle { case _ => Some(3) } Clump.handle { case _ => 3 }

Benefits:

  • Everything that was possible is still possible, ie. I'm not removing any functionality
  • API is simpler
  • No more need to use ClassTag hack to overload methods with Option[T] and T versions (1)
  • Some(None) no longer needed

(1) This is not technically true, orElse is still overloaded to accept both T and Clump[T]. To fix this, we'd have to come up with a different name for one of the methods. Perhaps orDefault?

@fwbrasil
Copy link
Contributor

fwbrasil commented Sep 7, 2015

@stevenheidel Awesome! We really don't need the overloaded methods receiving Option if there's Clump.empty.

👍 👍 👍 👍

@fwbrasil
Copy link
Contributor

fwbrasil commented Sep 7, 2015

(1) This is not technically true, orElse is still overloaded to accept both T and Clump[T]. To fix this, we'd have to come up with a different name for one of the methods. Perhaps orDefault?

👍 for renaming. It'd be a pattern similar to handle/rescue.

stevenheidel added a commit that referenced this pull request Sep 7, 2015
Simplify API to no longer expose internal Option
@stevenheidel stevenheidel merged commit d3c37c6 into getclump:master Sep 7, 2015
@williamboxhall
Copy link
Contributor

wow, this is awesome, really nice @stevenheidel

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