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

[ADAM-441] put a check in for Nothing. Throws an IAE if no return type is provided #535

Merged
merged 1 commit into from Jan 10, 2015

Conversation

Projects
None yet
4 participants
@calvertj
Contributor

calvertj commented Dec 24, 2014

Let me know if you want me to change anything. I also updated the
deprecated code and specified a return type for all implicits in
ADAMContext.scala

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Dec 24, 2014

Can one of the admins verify this patch?

Can one of the admins verify this patch?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Dec 24, 2014

Member

Jenkins, add to whitelist.

Member

fnothaft commented Dec 24, 2014

Jenkins, add to whitelist.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Dec 24, 2014

Member

Jenkins, test this please.

Member

fnothaft commented Dec 24, 2014

Jenkins, test this please.

sparkTest("sc.adamLoad should not load a file without a type specified") {
//load an existing file from the resources and save it as an ADAM file.
//This way we are not dependent on the ADAM format (as we would if we used a pre-made ADAM file)
//but we are dependent on the unmapped.sam file existing, maybe I should make a new one

This comment has been minimized.

@fnothaft

fnothaft Dec 24, 2014

Member

It should be fine to depend on unmapped.sam existing; if it disappears, this won't be the only test to fail ;)

@fnothaft

fnothaft Dec 24, 2014

Member

It should be fine to depend on unmapped.sam existing; if it disappears, this won't be the only test to fail ;)

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Dec 24, 2014

Member

+1, nifty work! I'll leave this open for a bit in case anyone else has comments, but LGTM.

Member

fnothaft commented Dec 24, 2014

+1, nifty work! I'll leave this open for a bit in case anyone else has comments, but LGTM.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Dec 24, 2014

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/516/
Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/516/
Test PASSed.

@@ -49,7 +49,7 @@ object ADAMContext {
implicit def sparkContextToADAMContext(sc: SparkContext): ADAMContext = new ADAMContext(sc)
// Add generic RDD methods for all types of ADAM RDDs
implicit def rddToADAMRDD[T <% SpecificRecord: Manifest](rdd: RDD[T]) = new ADAMRDDFunctions(rdd)
implicit def rddToADAMRDD[T](rdd: RDD[T])(implicit ev1: T => SpecificRecord, ev2: Manifest[T]): ADAMRDDFunctions[T] = new ADAMRDDFunctions(rdd)

This comment has been minimized.

@laserson

laserson Jan 10, 2015

Contributor

I'm a relative scala noob. Can you explain the significance of this change?

@laserson

laserson Jan 10, 2015

Contributor

I'm a relative scala noob. Can you explain the significance of this change?

This comment has been minimized.

@calvertj

calvertj Jan 10, 2015

Contributor

@laserson sure! He is my attempt to explain it.

View Bounds, for which the syntax is as follows with <% :

def myFun[A <% B](a: A) : B = a

basically mean that A can be treated like B. These are deprecated in 2.10, here is the original thread:
https://groups.google.com/forum/#!topic/scala-internals/hNow9MvAi6Q/discussion.
Instead we require that A must have an implicit method which converts A to B with the new syntax:

def myFun[A](a: A)(implicit ev1: A => B): B = a

ev1 here is short for evidence 1.
Here is a good article on it:
http://jatinpuri.com/2014/03/replace-view-bounds/
I think the newer syntax is clearer, but thats really the difference to us mere mortals, syntax.

  • edit, I fenced my code blocks
@calvertj

calvertj Jan 10, 2015

Contributor

@laserson sure! He is my attempt to explain it.

View Bounds, for which the syntax is as follows with <% :

def myFun[A <% B](a: A) : B = a

basically mean that A can be treated like B. These are deprecated in 2.10, here is the original thread:
https://groups.google.com/forum/#!topic/scala-internals/hNow9MvAi6Q/discussion.
Instead we require that A must have an implicit method which converts A to B with the new syntax:

def myFun[A](a: A)(implicit ev1: A => B): B = a

ev1 here is short for evidence 1.
Here is a good article on it:
http://jatinpuri.com/2014/03/replace-view-bounds/
I think the newer syntax is clearer, but thats really the difference to us mere mortals, syntax.

  • edit, I fenced my code blocks

This comment has been minimized.

@laserson

laserson Jan 10, 2015

Contributor

seems pretty clear...thanks!

@laserson

laserson Jan 10, 2015

Contributor

seems pretty clear...thanks!

This comment has been minimized.

@calvertj

calvertj Jan 10, 2015

Contributor

Glad to hear it, I rebased.

@calvertj

calvertj Jan 10, 2015

Contributor

Glad to hear it, I rebased.

@laserson

This comment has been minimized.

Show comment
Hide comment
@laserson

laserson Jan 10, 2015

Contributor

@calvertj Could you rebase this on master?

Contributor

laserson commented Jan 10, 2015

@calvertj Could you rebase this on master?

[ADAM-441] put a check in for Nothing. Throws an IAE if no return type
is provided.

Let me know if you want me to change anything.  I also updated the
deprecated code and specified a return type for all implicits in
ADAMContext.scala
@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Jan 10, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/529/
Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/529/
Test PASSed.

laserson added a commit that referenced this pull request Jan 10, 2015

Merge pull request #535 from calvertj/iss-441
[ADAM-441] put a check in for Nothing.  Throws an IAE if no return type is provided

@laserson laserson merged commit d3594fb into bigdatagenomics:master Jan 10, 2015

1 check passed

default Merged build finished.
Details
@laserson

This comment has been minimized.

Show comment
Hide comment
@laserson

laserson Jan 10, 2015

Contributor

Thanks, @calvertj!

Contributor

laserson commented Jan 10, 2015

Thanks, @calvertj!

@calvertj

This comment has been minimized.

Show comment
Hide comment
@calvertj

calvertj Jan 11, 2015

Contributor

My pleasure, I love the mission, and Scala, plus you guys have a nice code base. Hope I can find the time to do some more.

Contributor

calvertj commented Jan 11, 2015

My pleasure, I love the mission, and Scala, plus you guys have a nice code base. Hope I can find the time to do some more.

@calvertj calvertj deleted the calvertj:iss-441 branch Jan 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment