Adds UAST encoding options and improves tests

[ncordon]

• Lifts the formats from libuast native side to the managed layer.
• Secures way of passing format parameters and tree orders for iterators (we do not accept an incorrect number anymore).
• Improves tests for iterators: know table-driven, as @bzz requested and closer to python-client ones.
• Adds tests for encoding / decoding from the different formats.

Closes #107, closes #108

This change is 

[creachadair]

Reviewed 9 of 17 files at r1.
Reviewable status: 9 of 17 files reviewed, 2 unresolved discussions (waiting on @ncordon)

---

src/main/scala/org/bblfsh/client/v2/BblfshClient.scala, line 157 at r1 (raw file):

    * Since v2.
    */
  def decode(buf: ByteBuffer): ContextExt = Libuast.synchronized {

As written I think this is a breaking change to the API. Should we provide a default argument?

(I believe only DP team may be using this right now, but I'd really like to avoid breaking them without a good reason)

This also applies below.

---

src/main/scala/org/bblfsh/client/v2/package.scala, line 14 at r1 (raw file):

  import BblfshClient._

  /** Allow to use methods

I'm not sure why we would want to pun away the types here. Is this needed for some other interface we're relying on?

[creachadair]

Reviewed 4 of 17 files at r1.
Reviewable status: 13 of 17 files reviewed, 2 unresolved discussions (waiting on @ncordon)

[ncordon]

---

src/main/scala/org/bblfsh/client/v2/BblfshClient.scala, line 157 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

As written I think this is a breaking change to the API. Should we provide a default argument?

(I believe only DP team may be using this right now, but I'd really like to avoid breaking them without a good reason)

This also applies below.

No no, I have not touched the API at all. The syntax buf.decode() was not provided by these functions, but by an implicit here. implicits lie in memory and whenever I try to use buf.decode() Scala says: okay I do not know how to call decode on a ByteBuffer explicitly, do I have any implicits which tells me how to do it in my runtime? Here is some documentation for them: https://docs.scala-lang.org/overviews/core/implicit-classes.html

What I do not recommend is having default parameters in Scala, because they are not compatible with Java, as documented here. Of course we can use them if you prefer to do it. But that is why I have duplicated the decode methods for example into:

def decode(buf, format)
def decode(but) = decode(buf, UastBinary)

[ncordon]

---

src/main/scala/org/bblfsh/client/v2/package.scala, line 14 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

I'm not sure why we would want to pun away the types here. Is this needed for some other interface we're relying on?

I am only exposing the implicit conversions for TreeOrders and UastFormats. Let me explain a little bit. I did not want to break the API, because what we had until now was for example: def iterator(..., order: Int) or def decode(..., fmt:Int). Obviously if I introduced an integer which was not supported by libuast everything blew up. So I have turned those methods into iterator(..., order: TreeOrder) and decode(..., fmt: UastFormat), which are type-safe and would fail if I do not introduce a correct order / format. That being said, I wanted the old syntax iterator(..., 0) to work to avoid breaking the API, so I have to give Scala an implicit way of converting an integer to a correct TreeOrder (we use a default order warning the user when they introduce something invalid, like iterator(..., -1))

[creachadair]

Reviewed 4 of 17 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @creachadair)

---

src/main/scala/org/bblfsh/client/v2/BblfshClient.scala, line 157 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

No no, I have not touched the API at all. The syntax buf.decode() was not provided by these functions, but by an implicit here. implicits lie in memory and whenever I try to use buf.decode() Scala says: okay I do not know how to call decode on a ByteBuffer explicitly, do I have any implicits which tells me how to do it in my runtime? Here is some documentation for them: https://docs.scala-lang.org/overviews/core/implicit-classes.html

What I do not recommend is having default parameters in Scala, because they are not compatible with Java, as documented here. Of course we can use them if you prefer to do it. But that is why I have duplicated the decode methods for example into:

def decode(buf, format)
def decode(but) = decode(buf, UastBinary)

Understood, thanks for the explanation!

Given that this decode is to satisfy the interface, and isn't called directly by the client, I agree it doesn't make sense to add in defaults.

(As an aside: Implicit classes are a great example of a feature that really helps the author of the code, at the expense of the reader. I wish language designers would think about both sides more carefully.)

[ncordon]

---

src/main/scala/org/bblfsh/client/v2/package.scala, line 14 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

I am only exposing the implicit conversions for TreeOrders and UastFormats. Let me explain a little bit. I did not want to break the API, because what we had until now was for example: def iterator(..., order: Int) or def decode(..., fmt:Int). Obviously if I introduced an integer which was not supported by libuast everything blew up. So I have turned those methods into iterator(..., order: TreeOrder) and decode(..., fmt: UastFormat), which are type-safe and would fail if I do not introduce a correct order / format. That being said, I wanted the old syntax iterator(..., 0) to work to avoid breaking the API, so I have to give Scala an implicit way of converting an integer to a correct TreeOrder (we use a default order warning the user when they introduce something invalid, like iterator(..., -1))

I would rather erase this of course, but then we delete the backward compatibility with the old syntax passing numbers, and I prefer to have these things here. They are here because they would be in scope for the whole package, since treeOrders and uastFormats are used in several files from the library and I want the old syntax to work everywhere inside the package

[creachadair]

Reviewable status: complete! all files reviewed, all discussions resolved

---

src/main/scala/org/bblfsh/client/v2/package.scala, line 14 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

I would rather erase this of course, but then we delete the backward compatibility with the old syntax passing numbers, and I prefer to have these things here. They are here because they would be in scope for the whole package, since treeOrders and uastFormats are used in several files from the library and I want the old syntax to work everywhere inside the package

Makes sense, thanks for the clarification.

[creachadair]

Reviewable status: complete! all files reviewed, all discussions resolved

[creachadair]

Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

[ncordon]

---

src/main/scala/org/bblfsh/client/v2/BblfshClient.scala, line 157 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Understood, thanks for the explanation!

Given that this decode is to satisfy the interface, and isn't called directly by the client, I agree it doesn't make sense to add in defaults.

(As an aside: Implicit classes are a great example of a feature that really helps the author of the code, at the expense of the reader. I wish language designers would think about both sides more carefully.)

Yes, totally agree, when I started programming in Scala it was driving me mad constantly not knowing where the definitions for thing were because of the implicits stuff

[ncordon]

Just to clarify, tests are going to fail because in the file BblfshClientParseTest.scala I have included a test to check that encodeis the inverse method to decode, which right now is not true for YAML format. As explained, a list of roles like this:

Visibility, World, Declaration, Type

would be encoded as:

Declaration, Type, Visibility, World

due to this line which is code called by libuast. We are currently discussing about either sorting in decoding too or dropping that line.

[creachadair]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved