codacy / codacy-api-scala

Scala wrapper for the Codacy API
https://www.codacy.com
MIT License
6 stars 5 forks source link

CodacyClient uses Play Json to parse response - creates conflict in projects that use a different version of Play Framework #3

Closed DougC closed 7 years ago

DougC commented 8 years ago

I have a project using Play 2.5 into which I am trying to integrate sbt-codacy-coverage. Running abt codacyCoverage results in the exception below. This is because codacy-api-scala depends on Play 2.3 and the Json implementation in 2.5 is incompatible with the 2.3 implementation.

The coverage data does, in fact, get uploaded to codacy, and the exception occurs when processing the result. This makes my circle-ci build fail.

java.lang.NoSuchMethodError: play.api.libs.json.JsValue.$bslash(Ljava/lang/String;)Lplay/api/libs/json/JsValue; at com.codacy.api.client.CodacyClient.com$codacy$api$client$CodacyClient$$parseJson(CodacyClient.scala:98) at com.codacy.api.client.CodacyClient$$anonfun$post$1.apply(CodacyClient.scala:65) at com.codacy.api.client.CodacyClient$$anonfun$post$1.apply(CodacyClient.scala:53) at com.codacy.api.client.CodacyClient.withWSClient(CodacyClient.scala:108) at com.codacy.api.client.CodacyClient.post(CodacyClient.scala:53) at com.codacy.api.service.CoverageServices.sendReport(CoverageServices.scala:14) at com.codacy.CodacyCoveragePlugin$$anonfun$1.apply(CodacyCoveragePlugin.scala:61) at com.codacy.CodacyCoveragePlugin$$anonfun$1.apply(CodacyCoveragePlugin.scala:48)

pedrorijo91 commented 8 years ago

Hey @DougC

We are aware of our projects dependency to Play specific versions (typically Play 2.4.x), that cause problems when using other Play versions (eg. https://github.com/codacy/sbt-codacy-coverage/issues/14)

We have been trying to find time to solve this problem, but unfortunately we haven't been able to do it until now, and we cannot predict when will we solve this issue for now.

We will try to push this, in the meanwhile any PR is welcome and very appreciated

DougC commented 8 years ago

I would suggest moving away from Play WS and Json implementations to other libs. Although they are very convenient libraries, you do end up pulling pretty much all of Play framework to use them and they cause this sort of version conflict. Using other implementations might also result in conflicts with projects that use different versions, but it might be less likely, especially as Play does update frequently and often does break compatibility.

Of course, the real issue lies with SBT not isolating the plugin dependencies from the project dependencies, but I'm not sure if that's ever going to change.

If I get a chance, I might fork this lib and re-implement using other http and json libs, if you think that would be helpful.

pedrorijo91 commented 8 years ago

We want to move away from Play WS and Json, as you mention. Several internal projects are already using other libs but, as I said, we hadn't the time to migrate all our projects.

If you could get some time to contribute it would be great :)

DougC commented 8 years ago

If you have preferred implementations let me know, otherwise I'll choose some myself :-)

pedrorijo91 commented 8 years ago

for json we have been using rapture, for http feel free to choose one :)

DougC commented 8 years ago

Ok, I'll use that. I see rapture also has an http client library, which is handy :-)

pedrorijo91 commented 8 years ago

fell free to ping us if you need any help from our side 😃

DougC commented 8 years ago

Are you aware of any clients of codacy-api-scala other than sbt-codacy-coverage? The issue is that the CodacyClient methods are very dependent on Play JSON (because of the implicit Reads[T] parameter) so changing to any other Json implementation will break those method signatures.

rtfpessoa commented 8 years ago

I am not aware of any major uses of this. Do not worry about compatibility.

We can release a major version.

pedrorijo91 commented 8 years ago

Hey @DougC ,

How's everything going? Do you need any help removing the dependencies, or are you just lacking the time?

DougC commented 8 years ago

Hi Pedro,

It's mostly the time thing. I've actually made most of the changes, which you can see here https://github.com/WellFactored/codacy-api-scala/tree/remove-play-dependency, but my testing showed that the JSON serialisation was not working properly and I haven't had a chance to go back and fix it yet (I got thrown onto a new project at work which is occupying much of my time at the moment)

I chose to use Dispatch for the http requests (as it's a pretty straightforward client) and json4s for the json handling. I had a look at Rapture JSON but found it over-complicated for the needs of the project and poorly documented. I also considered Circe, but decided against it as it has dependencies on cats and shapeless that I didn't want to pull into an sbt plugin as they're likely to create the same sort of conflicts with the client project as we're seeing with Play.

My approach has been to introduce a type class, BodyFormat[T] that abstracts away the JSON handling and just describes a way to convert a T to and from a String for use as an http body. I've then implemented BodyFormats based on Json4s. This means that the CodacyClient api's methods can specify that they need BodyFormat instances and we don't need to leak to the client any information about how we're handling json conversion internally (and, in fact, they can use their own json client if they want). This should give us some future-proffing against changing json clients in the future.

The other breaking change I've made is to convert Language from a scala Enumeration to be a sealed trait with case objects. This is purely down to my preference for not using Enumeration (as being inherently non-scala-ish) and if it's a big issue for you then I'm happy to revert it.

I hope to be able to get back to this and figure out why my Json4s implementation is not working soon and finish it up.