Switch to proto-lens #296

tclem · 2019-09-30T21:38:30Z

Fixes #89

Move away from our custom fork of proto3-* and use proto-lens instead. There are still some missing pieces here wrt to JSONPB encoding/decoding and the protobuf tooling setup that I'm working through.

robrix · 2019-10-02T14:25:07Z

src/Semantic/Api/Terms.hs

@@ -87,7 +88,7 @@ data TermOutputFormat
 parseTermBuilder :: (Traversable t, Member Distribute sig, ParseEffects sig m, MonadIO m)
  => TermOutputFormat -> t Blob -> m Builder
 parseTermBuilder TermJSONTree    = distributeFoldMap jsonTerm >=> serialize Format.JSON -- NB: Serialize happens at the top level for these two JSON formats to collect results of multiple blobs.
-parseTermBuilder TermJSONGraph   = termGraph >=> serialize Format.JSONPB
+parseTermBuilder TermJSONGraph   = termGraph >=> serialize Format.JSON


⚠️ there will be conflicts with #300; hopefully pretty minor, but let me know if you’d like another pair of 👀 on anything.

robrix

Some notes/comments. Good stuff!

robrix · 2019-10-02T21:45:56Z

cabal.project

-  location: https://github.com/joshvera/proto3-wire.git
-  tag: 84664e22f01beb67870368f1f88ada5d0ad01f56
+  location: https://github.com/tclem/proto-lens-jsonpb
+  tag: 2b4cd579144ea10c3bd36bd05122c233dc5334d7


robrix · 2019-10-02T21:46:59Z

semantic.cabal

@@ -304,6 +304,9 @@ library
                     , prettyprinter ^>= 1.2.1
                     , pretty-show ^>= 1.9.5
                     , profunctors ^>= 5.3
+                     , proto-lens == 0.5.1.0
+                     , proto-lens-jsonpb


Do we need a version bound on this?

Should these version bounds be using ^>= to express “up to the next major version”?

Upon reflection we probably don’t need a version bound on proto-lens-jsonpb because we’re bringing it in with source-repository-package, but I think ^>= for proto-lens is a good idea either way.

We’ll eventually want to push proto-lens-jsonpb to Hackage so we can fix #16, so a version bound on both these is good to me.

robrix · 2019-10-02T21:49:41Z

src/Rendering/Graph.hs

-    Merge t@(In (a1, a2) syntax)     -> diffAlgebra t  (Merged   (Just (MergedTerm (T.pack (constructorName syntax)) (ann a1) (ann a2))))
-    Patch (Delete t1@(In a1 syntax)) -> diffAlgebra t1 (Deleted  (Just (DeletedTerm (T.pack (constructorName syntax)) (ann a1))))
-    Patch (Insert t2@(In a2 syntax)) -> diffAlgebra t2 (Inserted (Just (InsertedTerm (T.pack (constructorName syntax)) (ann a2))))
+    Merge t@(In (a1, a2) syntax)     -> diffAlgebra t . DiffTreeVertex'Merged $ defMessage


I’m having a lot of trouble understanding the control flow here, in part due to the combined use of $ and &. Can you check my understanding, please? To wit: we’re starting with the default message, setting a bunch of fields on it with the & … lines below, and finally wrapping it up in the DiffTreeVertex'Merged constructor and passing that to the algebra. Is that right?

If so, I’m not sure I really love that idiom relative to using constructors directly, but perhaps that’s just not the way this approach works…?

Yeah, it’s… it’s not the world’s most natural idiom in Haskell. But avoiding constructors and updating a default value through lenses lets you hide implementation details such as the _unknownFields datum attached to message types, which gracefully handles the presence of fields we don’t recognize or can’t decrypt. (proto3-suite just drops that information on the ground silently).

Remember that in protobuf records, every type is, essentially, an instance of Lower—given a message, every message field inside it is a Maybe defaulting to Nothing, every string field defaults to “”, and every numeric field defaults to 0. This means there’s always a sensible defMessage (similar in essence to lowerBound).

Yeah, excellent points all around. I wouldn’t mind if we could use a (smart) constructor (ideally generated) that would include all the fields except for _unknownFields, tho, e.g.:

thing :: Foo -> Bar -> Thing thing foo bar = defMessage & P.foo .~ foo & P.bar .~ bar

Guessing I should file that upstream?

Remember that in protobuf records, every type is, essentially, an instance of Lower

This is such a good way of describing this ☝️ 👏

Upon reflection, it occurs to me that it’s probably a good thing that the idiom for the proto-lens code is substantially different than that for our typical internal types. Tho it does kind of reinforce my suspicion that this is a hint that this code should be working on internal types if possible which we bridge to wire types. (Tho that would involve significant restructuring to handle the JSON formatting of the wire types, so maybe I should just shush and let you get on with things.)

Either way, I think any of the above that we decide to tackle can wait for future PRs. This is great work, @tclem.

robrix · 2019-10-02T21:50:49Z

src/Rendering/Graph.hs

+                                          & P.maybe'span .~ ann a1
+    Patch (Insert t2@(In a2 syntax)) -> diffAlgebra t2 . DiffTreeVertex'Inserted $ defMessage
+                                          & P.term .~ T.pack (constructorName syntax)
+                                          & P.maybe'span .~ ann a2


I’m additionally wondering if this is a hint that we should be producing some internal, intermediate type that we bridge to the wire types separately. What do you think?

I think that's a very good direction to move in general.

I think these graph representations essentially are a translation of internal types to output types that are suitable for serialization or display.

Yeah, that’s a good point. I think superficially I was meaning “move this lens code somewhere else” but a) that’s a different story altogether, b) it looks like it might be super inconvenient, and c) I really don’t think we ought to hold up this PR just for that.

robrix · 2019-10-02T21:51:10Z

src/Rendering/Graph.hs

-        let root = vertex (DiffTreeVertex (fromIntegral i) (Just a))
+        let root = vertex $ defMessage
+              & P.diffVertexId .~ fromIntegral i
+              & P.maybe'diffTerm ?~ a


I’m going to have to learn to read lens, I guess.

tclem · 2019-10-03T16:02:55Z

src/Proto/Semantic_JSON.hs

@@ -0,0 +1,842 @@
+-- Code generated by protoc-gen-jsonpb_haskell 0.1.0, DO NOT EDIT.


proto-lens doesn't currently support encoding/decode to json so this is a bit of a hack to preserve semantic's existing API and functionality. This file gets generated by https://github.com/tclem/proto-lens-jsonpb, a protoc plugin and haskell library for encoding/decoding jsonpb which I ported over from proto3-suite. See https://developers.google.com/protocol-buffers/docs/proto3#json for how protobufs are suppose to translate to json.

tclem · 2019-10-03T16:04:36Z

src/Proto/Semantic_JSON.hs

+    [ "service" .= (x^.service)
+    ]
+
+instance FromJSON PingRequest where


These are all orphan instances. A future alternative would be to contribute something to proto-lens so that this code is generated with the proto-lens-protoc plugin, but I'll leave that for another day.

tclem · 2019-10-03T16:07:17Z

src/Rendering/Graph.hs

+                                          & P.maybe'span .~ ann a1
+    Patch (Insert t2@(In a2 syntax)) -> diffAlgebra t2 . DiffTreeVertex'Inserted $ defMessage
+                                          & P.term .~ T.pack (constructorName syntax)
+                                          & P.maybe'span .~ ann a2


I think that's a very good direction to move in general.

I think these graph representations essentially are a translation of internal types to output types that are suitable for serialization or display.

patrickt

Amazing work, @tclem!

robrix

Nice work @tclem!

robrix · 2019-10-03T16:52:15Z

.gitignore

@@ -24,6 +24,7 @@ tmp/
 *.hp
 *.prof
 *.pyc
+/hie.yaml


Been looking forward to merging just for this 😉

robrix · 2019-10-03T16:52:33Z

cabal.project

-  location: https://github.com/joshvera/proto3-wire.git
-  tag: 84664e22f01beb67870368f1f88ada5d0ad01f56
+  location: https://github.com/tclem/proto-lens-jsonpb
+  tag: e4d10b77f57ee25beb759a33e63e2061420d3dc2


Is this something we can/should push to hackage at some point?

We definitely can, I'm not sure we should just yet. Let's see if this approach works for us.

robrix · 2019-10-03T16:54:30Z

src/Semantic/Api/Diffs.hs

+          & P.path .~ path
+          & P.language .~ lang
+          & P.vertices .~ mempty
+          & P.edges .~ mempty


Is it generally the case that these fields would be default-initialized to the same value as mempty?

Yes, we could remove the setting to mempty entirely.

tclem added 9 commits September 27, 2019 16:42

First pass at switching from proto3 to proto-lens

c0cce3b

Already are using lens

06a108a

Bump to newer version of proto-lens

c0a53e9

Merge remote-tracking branch 'origin/master' into proto-lens

7002b3c

Merge remote-tracking branch 'origin/master' into proto-lens

d678f5b

Fix a few tests

e3506e8

Try out manually defining json instances

cba98ae

Write out custom To/FromJSON instances

fccbe6a

Thanks @patrickt

6488e6e

robrix reviewed Oct 2, 2019

View reviewed changes

tclem added 4 commits October 2, 2019 13:14

Use tclem/proto-lens-jsonpb

85bf7e7

Fix up tests

3fe174a

Merge remote-tracking branch 'origin/master' into proto-lens

5fbcca4

Don't need to hide readFile

1232cc3

tclem changed the title ~~WIP: Switch to proto-lens~~ Switch to proto-lens Oct 2, 2019

Minor cleanup

54eb044

tclem requested a review from a team October 2, 2019 21:36

robrix approved these changes Oct 2, 2019

View reviewed changes

++proto-lens-jsonpb to handle excluding default fields

32f6e9d

tclem commented Oct 3, 2019

View reviewed changes

tclem added 2 commits October 3, 2019 09:09

Allow up to next major version for proto-lens

c537122

Merge remote-tracking branch 'origin/master' into proto-lens

f8c76b2

patrickt approved these changes Oct 3, 2019

View reviewed changes

robrix approved these changes Oct 3, 2019

View reviewed changes

tclem merged commit 05edc30 into master Oct 3, 2019

tclem deleted the proto-lens branch October 3, 2019 17:15

This was referenced Oct 4, 2019

API.DiffTreeVertexDiffTerm should not be a sum type. #168

Closed

Use Hackage’s proto3-suite. #111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to proto-lens #296

Switch to proto-lens #296

tclem commented Sep 30, 2019

robrix Oct 2, 2019

robrix left a comment

robrix Oct 2, 2019

robrix Oct 2, 2019

robrix Oct 3, 2019

patrickt Oct 3, 2019

robrix Oct 2, 2019

robrix Oct 2, 2019

patrickt Oct 3, 2019

robrix Oct 3, 2019

robrix Oct 3, 2019

robrix Oct 2, 2019

tclem Oct 3, 2019

robrix Oct 3, 2019

robrix Oct 2, 2019

tclem Oct 3, 2019

tclem Oct 3, 2019

tclem Oct 3, 2019

patrickt left a comment

robrix left a comment

robrix Oct 3, 2019

robrix Oct 3, 2019

tclem Oct 3, 2019

robrix Oct 3, 2019

tclem Oct 3, 2019 •

edited

Loading

		@@ -0,0 +1,842 @@
		-- Code generated by protoc-gen-jsonpb_haskell 0.1.0, DO NOT EDIT.

Switch to proto-lens #296

Switch to proto-lens #296

Conversation

tclem commented Sep 30, 2019

Choose a reason for hiding this comment

robrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickt left a comment

Choose a reason for hiding this comment

robrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tclem Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

tclem Oct 3, 2019 •

edited

Loading