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

Moves to 2.10.x as the default and to scalatest #176

Merged
merged 10 commits into from
Aug 29, 2014

Conversation

ianoc
Copy link
Collaborator

@ianoc ianoc commented Aug 29, 2014

No description provided.

@@ -58,7 +58,7 @@ object GenericAvroCodecsSpecification extends Specification with BaseProperties
val testRecord = buildGenericAvroRecord(("2012-01-01", 1, 12))
val bytes = Injection[GenericRecord, Array[Byte]](testRecord)
val attempt = Injection.invert[GenericRecord, Array[Byte]](bytes)
attempt.get must_== testRecord
attempt.get shouldEqual testRecord
Copy link
Contributor

Choose a reason for hiding this comment

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

you should check out the assert macro and diagrammatic asserts, it's incredibly cool.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI for anyone else who sees this: http://www.artima.com/docs-scalatest-2.2.0-RC1/index.html#org.scalatest.DiagrammedAssertions

Those look very slick. I'll swap all these shouldEqual over to asserts to get the base info. Looks like we can flick those on in future for more info

…an use the assert macro's from scalatest and also get better combined test output info.
@ianoc
Copy link
Collaborator Author

ianoc commented Aug 29, 2014

This turned into a little more monsterish than I originally intended in size. But all warnings are gone from the code under 2.10, all manifests are gone. Also all test entry points are now scalatest, using its wrappers to do scalacheck tests. Gives a more common platform for tests imo. Thought's @johnynek ? There are a bunch of pro's and cons with certain parts getting longer or shorter between the two. But one nice thing is getting scalatests macro's, we have some horrible hacks of do a list that makes bools and then do a forall at the end of x => x. With the asserts instead we will get more info at the call site.

I also nuked the old JavaTests.scala, I confirmed under scalatest the Javatests are being run anyway.

@@ -41,6 +41,7 @@ trait Bufferable[T] extends Serializable {
get(from) match {
case Success(tup @ _) => tup
case Failure(InversionFailure(_, t)) => throw t
case Failure(t) => throw t
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice catch (or, nice throw!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the warnings housekeeping tossed it up, also just a an interesting one: json4s needs a manifest, though in our code we can supply the call site with a TypeTag and ClassTag and scala has an implicit conversion to a manifest for it.

@johnynek
Copy link
Collaborator

shipit

ianoc added a commit that referenced this pull request Aug 29, 2014
Moves to 2.10.x as the default and to scalatest
@ianoc ianoc merged commit 23b7978 into develop Aug 29, 2014
@ianoc ianoc deleted the bijection-scalatest_2.10 branch August 29, 2014 18:50
def toJson[T <: SpecificRecordBase: Manifest](schema: Schema): Injection[T, String] = {
val klass = manifest[T].erasure.asInstanceOf[Class[T]]
def toJson[T <: SpecificRecordBase: ClassTag](schema: Schema): Injection[T, String] = {
val klass = classTag[T].runtimeClass.asInstanceOf[Class[T]]
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to just have a method in the package which is the following:

def klass[T](implicit ct: ClassTag[T]): Class[T] = ct.runtimeClass.asInstanceOf[Class[T]]

Not a big deal at all but could get rid of the boilerplate at least.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good to me.

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.

None yet

4 participants