Adding missing fields to User

[mfirry]

I needed these two fields for a personal project (though I know they're pretty easy to come up with... nonetheless, feel free to reject this should you think it's not required.

Note: I had to explicitly define circe's en/decoders on User's companion object and use semiauto instead of auto in Users. Not sure why I had to but I kept having weird DerivedDecoder issues without this change.

[JesusMtnez]

@mfirry Thank you for this contribution. At first, it looks good.
Did you try to run the tests? I'm not sure if you may need to update the tests too. I cannot try them now, but I could do it later.

[BenFradet]

I had to explicitly define circe's en/decoders on User's companion object and use semiauto instead of auto in Users. Not sure why I had to but I kept having weird DerivedDecoder issues without this change.

Weird, what kind of issue were you getting?

[mfirry]

So at first I figured I could "just" add the two fields (both as Option[String]). I did that and published it locally and tried it (as a dep of my project) and I had the following:

(it's a Play app)

play.api.http.HttpErrorHandlerExceptions$$anon$1: Execution exception[[RuntimeException: java.lang.NoSuchMethodError: github4s.free.domain.User.<init>(ILjava/lang/String;Ljava/lang/String;Ljava/lang/String;Lscala/Option;Lscala/Option;Lscala/Option;Lscala/Option;Lscala/Option;Lscala/Option;)V]]
  at play.api.http.HttpErrorHandlerExceptions$.throwableToUsefulException(HttpErrorHandler.scala:323)
  at play.api.http.DefaultHttpErrorHandler.onServerError(HttpErrorHandler.scala:243)
  at play.core.server.AkkaHttpServer$$anonfun$1.applyOrElse(AkkaHttpServer.scala:382)
  at play.core.server.AkkaHttpServer$$anonfun$1.applyOrElse(AkkaHttpServer.scala:380)
  at scala.concurrent.Future.$anonfun$recoverWith$1(Future.scala:417)
  at scala.concurrent.impl.Promise.$anonfun$transformWith$1(Promise.scala:41)
  at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
  at akka.dispatch.BatchingExecutor$AbstractBatch.processBatch(BatchingExecutor.scala:55)
  at akka.dispatch.BatchingExecutor$BlockableBatch.$anonfun$run$1(BatchingExecutor.scala:91)
  at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
Caused by: java.lang.RuntimeException: java.lang.NoSuchMethodError: github4s.free.domain.User.<init>(ILjava/lang/String;Ljava/lang/String;Ljava/lang/String;Lscala/Option;Lscala/Option;Lscala/Option;Lscala/Option;Lscala/Option;Lscala/Option;)V
  at play.api.mvc.ActionBuilder$$anon$10.apply(Action.scala:425)
  at play.api.mvc.Action.$anonfun$apply$2(Action.scala:97)
  at play.api.libs.streams.StrictAccumulator.$anonfun$mapFuture$4(Accumulator.scala:175)
  at scala.util.Try$.apply(Try.scala:213)
  at play.api.libs.streams.StrictAccumulator.$anonfun$mapFuture$3(Accumulator.scala:175)
  at scala.Function1.$anonfun$andThen$1(Function1.scala:57)
  at scala.Function1.$anonfun$andThen$1(Function1.scala:57)
  at scala.Function1.$anonfun$andThen$1(Function1.scala:57)
  at play.api.libs.streams.StrictAccumulator.run(Accumulator.scala:208)
  at play.core.server.AkkaHttpServer.$anonfun$runAction$4(AkkaHttpServer.scala:376)
Caused by: java.lang.NoSuchMethodError: github4s.free.domain.User.<init>(ILjava/lang/String;Ljava/lang/String;Ljava/lang/String;Lscala/Option;Lscala/Option;Lscala/Option;Lscala/Option;Lscala/Option;Lscala/Option;)V
  at github4s.api.Users$anon$importedDecoder$macro$25$1$anon$macro$23$1.from(Users.scala:45)
  at github4s.api.Users$anon$importedDecoder$macro$25$1$anon$macro$23$1.from(Users.scala:45)
  at shapeless.LabelledGeneric$$anon$1.from(generic.scala:229)
  at shapeless.LabelledGeneric$$anon$1.from(generic.scala:226)
  at io.circe.generic.decoding.DerivedDecoder$$anon$1.apply(DerivedDecoder.scala:15)
  at io.circe.Decoder.decodeJson(Decoder.scala:64)
  at io.circe.Decoder.decodeJson$(Decoder.scala:64)
  at io.circe.generic.decoding.DerivedDecoder.decodeJson(DerivedDecoder.scala:6)
  at io.circe.Parser.finishDecode(Parser.scala:13)
  at io.circe.Parser.finishDecode$(Parser.scala:9)
[info] p.c.s.AkkaHttpServer - Stopping server...

[pepegar]

Hi @mfirry.

I believe you're having a binary compatibility issue between the library that's currently published (0.20.0), and the one you've published locally.

The error you're receiving says that the current github4s artifact in your classpath doesn't have all those fields.

Can you share your libraryDependencies value to see if there's any issue there?

you could try doing a sbt clean, followed by bumping github4s version & releasing locally, followed by depending on the new locally published dependency in your play app. That should work?

[mfirry]

Hi @pepegar,
"com.47deg" %% "github4s" % "0.20.1-SNAPSHOT" is on my deps list.

I'll try what you suggest and update this PR if it works.

Thanks a lot.

[mfirry]

Looks like you were right @pepegar. I'll update this in a minute.

[BenFradet]

successfully ran the tests locally 👍

[mfirry]

I might need to add more fields (like type, hireable, public_repos and so on...).

Should I do this in one big PR (this one) or smaller ones?

[pepegar]

@mfirry regarding adding more fields, I don't have a strong opinion. If you want to keep adding commits to this PR is fine from my side. Just mark this PR as WIP and ping us when you're done.

[mfirry]

I'm re-thinking about the following and followers part... I might add getFollowing and getFollowers to the User algebra. I'll make a quick try and let you guys know.

Having a url as a String field of the User does not seem very useful.

[mfirry]

Ok it does work but the User object in the /following call is slightly different than the one in the /users/_username_ (for example it misses the public_repos field).

How to go about it?

• having lots of optional fields
• define another User case class (think LightUser) with less fields

I'll spend the weekend thinking about.

What's your take?

[pepegar]

Hi @mfirry,

hum... that's tricky :)

How many different fields are there? My gut feeling is that, if there's no a lot of different fields I prefer to keep a single entity User.

If the fields change a lot though, it may make sense to have different entities.

Sorry to give you such a vague answer :)

I think we could discuss it much better if you can push your WIP to see how many optional fields we end up with.

[mfirry]

To answer your question, here's the list of fields you get from the users api and you don't get from the following one:

<   "name": "Tom Preston-Werner",
<   "company": null,
<   "blog": "http://tom.preston-werner.com",
<   "location": "San Francisco",
<   "email": null,
<   "hireable": null,
<   "bio": null,
<   "public_repos": 61,
<   "public_gists": 62,
<   "followers": 21344,
<   "following": 11,
<   "created_at": "2007-10-20T05:24:19Z",
<   "updated_at": "2018-12-18T19:52:12Z"

Of all of these, in this PR, I've added hirebale, bio and public_repos...
Here's what I figured I could do: I'm gonna set public_repos as Option[Int] and end this one here.

I'll open a separate PR in order to be able to add the GetFollowing to the Users ops ADT.

Deal? :)

[mfirry]

Hi. I'm pretty much done here. Let me know if it makes sense to you. Cheers!

[pepegar]

Hey @mfirry, sorry I forgot about this. I'll review it ASAP, thank you!

[Yawolf]

Hi @mfirry, thanks for your contribution! I think your changes are good! However you have to fix also the test so we can have a green in travis and merge your PR 🙂

Fix the tests so we can fully review you PR!!

Thanks! 😄

[mfirry]

Uh sorry. My bad. Don't know how that happened. Fixing it as we speak.

[mfirry]

So some integration tests fail... but it's not clear to me whether I can do anything about it. Can somebody else have a look and point me to the right direction?
Thanks

[juanpedromoreno]

Thanks @mfirry , I ran the tests locally and they succeeded