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 a couple inferred methods and aliases to clump api #88

Merged
merged 7 commits into from
Apr 24, 2015

Conversation

stevenheidel
Copy link
Contributor

The most useful of these to me is default. I want to be able to go:

for {
  list <- clumpSourceThatReturnsList.get(id).default(Nil)
} yield {
  // code
}

instead of having to do this all the time:

for {
  listOption <- clumpSourceThatReturnsList.get(id).optional
} yield {
  val list = listOption.getOrElse(Nil)
  // code
}

@williamboxhall
Copy link
Contributor

Interesting point @stevenheidel.

Do you think it could be easier to use

clumpSourceThatReturnsList.get(id).orElse(Clump.value(Nil))

Or if you don't like wrapping up the Nil in a clump every time, maybe it would be good to have a getOrElse on the ClumpSource interface?

clumpSourceThatReturnsList.getOrElse(id, Nil)

/**
* Alias for [[value]]
*/
def apply[T](value: T): Clump[T] = this.value(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep a behavior similar to Future.apply[0], this method should propagate exceptions inside a Clump instance. Suggestion:

def apply[T](value: => T): Clump[T] = this.future(Future(value))

Same observation for the next method.

[0] https://github.com/scala/scala/blob/2.11.x/src/library/scala/concurrent/Future.scala#L492

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@stevenheidel
Copy link
Contributor Author

@williamboxhall - clumpSourceThatReturnsList.get(id).orElse(Clump.value(Nil)) works, but it's kind of silly to wrap the Nil in a Clump. I supposed that's made slightly easier by the .apply method so you could write clumpSourceThatReturnsList.get(id).orElse(Clump(Nil))

I like your suggestion for a .getOrElse though.

@stevenheidel
Copy link
Contributor Author

This is what .getOrElse would look like on ClumpSource:

  def getOrElse(input: T, default: => U): Clump[U] =
    get(input).orElse(Clump(default))

@fwbrasil
Copy link
Contributor

About Clump.value vs Clump[Option[T]]. The usage of an option inside a Clump instance is a feature for corner cases where we want to avoid lossy joins. The Clump result is already optional. Given this, I think the current alternative is satisfactory:

val clump: Clump[Option[T]] = Clump.value(someValue).optional

@fwbrasil
Copy link
Contributor

@williamboxhall I wouldn't like to pollute the ClumpSource interface with transformation methods (getOrElse). I think the Clump's interface should be expressive enough so we have only one way of applying transformations.

@stevenheidel
Copy link
Contributor Author

@fwbrasil - thanks, renamed default to orElse using ClassTag.

@fwbrasil
Copy link
Contributor

@stevenheidel Thanks, could you add tests for the new methods?

@stevenheidel
Copy link
Contributor Author

@fwbrasil - Added tests for fallback, fallbackTo, and orElse. How would you like me to test the methods that are just synonyms? I don't really want to just copy the tests from the other methods.

@fwbrasil
Copy link
Contributor

@stevenheidel Please add tests for all methods, we aim to keep the 100% code coverage.

@stevenheidel
Copy link
Contributor Author

@fwbrasil - For the tests that are just synonyms of other methods do you want me to just copy the tests?

@fwbrasil
Copy link
Contributor

@stevenheidel The test needs to check that the synonyms are pointing to the correct methods and passing the correct params. It is ok to copy an existing test if it tests the synonym properly.

@fwbrasil
Copy link
Contributor

Thanks @stevenheidel!

👍 cc/ @williamboxhall

@@ -47,6 +67,11 @@ sealed trait Clump[+T] {
def filter[B >: T](f: B => Boolean): Clump[B] = new ClumpFilter(this, f)

/**
* If this clump does not return a value then use the default instead
*/
def orElse[B >: T: ClassTag](default: => B): Clump[B] = new ClumpOrElse(this, Clump.value(default))
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@williamboxhall
Copy link
Contributor

👍 some nice additions

fwbrasil added a commit that referenced this pull request Apr 24, 2015
Add a couple inferred methods and aliases to clump api
@fwbrasil fwbrasil merged commit d0799f9 into getclump:master Apr 24, 2015
@stevenheidel stevenheidel deleted the moremethods branch June 4, 2015 21:58
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