Field comments #172

clavoie-takt · 2018-01-19T22:25:19Z

Revive @judah 's old field-comments branch, and prettify the output a wee bit.

This change is

…eir long-form variants.

googlebot · 2018-01-19T22:25:24Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

clavoie-takt · 2018-01-19T22:35:36Z

CLA ping -- bot should now rubberstamp.

googlebot · 2018-01-19T22:35:41Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

judah · 2018-01-19T22:42:34Z

Confirming that I am OK with my commit being used here.

judah

A couple small comments; also, please take a look at the two errors in CI (let me know if you have trouble reproducing them): https://travis-ci.org/google/proto-lens/builds/331017983?utm_source=github_status&utm_medium=notification

judah · 2018-01-22T17:21:52Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs

@@ -28,7 +28,10 @@ import Data.String (fromString)
 import Data.Text (unpack)
 import qualified Data.Text as T
 import Data.Tuple (swap)
+import qualified Language.Haskell.Exts.Syntax as Syntax


Don't import this module directly; we're intentionally using the Combinators module as a shim. If necessary, add more exports to that module instead.

In particular, you can get ModuleName from it: https://github.com/google/proto-lens/blob/master/proto-lens-protoc/src/Data/ProtoLens/Compiler/Combinators.hs#L218

judah · 2018-01-22T17:23:39Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs

-messageComment :: Name -> [RecordField] -> String
-messageComment n fields = unlines
+messageComment :: Syntax.ModuleName () -> Name -> [RecordField] -> String
+messageComment (Syntax.ModuleName _ fieldModName) n fields = unlines


ModuleName is an instance of Pretty, so I believe you can just use prettyPrint here instead of unwrapping the constructor.

clavoie-takt · 2018-01-22T18:18:10Z

Fixed all.

…

On Mon, Jan 22, 2018 at 12:27 PM, Judah Jacobson ***@***.***> wrote: ***@***.**** requested changes on this pull request. A couple small comments; also, please take a look at the two errors in CI (let me know if you have trouble reproducing them): https://travis-ci.org/google/proto-lens/builds/331017983? utm_source=github_status&utm_medium=notification ------------------------------ In proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs <#172 (comment)>: > @@ -28,7 +28,10 @@ import Data.String (fromString) import Data.Text (unpack) import qualified Data.Text as T import Data.Tuple (swap) +import qualified Language.Haskell.Exts.Syntax as Syntax Don't import this module directly; we're intentionally using the Combinators module as a shim. If necessary, add more exports to that module instead. In particular, you can get ModuleName from it: https://github.com/google/ proto-lens/blob/master/proto-lens-protoc/src/Data/ProtoLens/Compiler/ Combinators.hs#L218 ------------------------------ In proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs <#172 (comment)>: > @@ -165,15 +168,16 @@ reexported ***@***.*** {importModule = m} where m' = fromString $ "Data.ProtoLens.Reexport." ++ prettyPrint m -messageComment :: Name -> [RecordField] -> String -messageComment n fields = unlines +messageComment :: Syntax.ModuleName () -> Name -> [RecordField] -> String +messageComment (Syntax.ModuleName _ fieldModName) n fields = unlines ModuleName is an instance of Pretty, so I believe you can just use prettyPrint here instead of unwrapping the constructor. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#172 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AfCrwmP7yQGTGfLWshGCWXS2X-RUsARUks5tNMTugaJpZM4RlIf7> .

-- Christian Lavoie christian.lavoie@takt.com

Now that we define our own `Module` type (google#172), we don't need `getModuleName` to return a `Maybe`.

Now that we define our own `Module` type (#172), we don't need `getModuleName` to return a `Maybe`.

They're not that useful for constructing values (especially after adding the "unknown fields" case) and were previously mostly used for documentation. Now that google#172 has landed, we have a better source of documentation that doesn't expose internal details. Also change the "Show" instance for messages to be a wrapper around `showMessageShow`. This both hides the now-inaccessible record fields and shortens the output in most cases. For example: ```> print (def :: DescriptorProto) {} > print (def & #name .~ "hello" & #field .~ [def, def & #name .~ "bye"]:: DescriptorProto) {name: "hello" field { } field { name: "bye" }} ``` Also bump the resolver for bootstrapping to match the regular build. Hide message constructors. showsPrec, and bootstrap

They're not that useful for constructing values (especially after adding the "unknown fields" case) and were previously mostly used for documentation. Now that google#172 has landed, we have a better source of documentation that doesn't expose internal details. Also change the "Show" instance for messages to be a wrapper around `showMessageShow`. This both hides the now-inaccessible record fields and shortens the output in most cases. For example: ``` > print (def :: DescriptorProto) {} > print (def & #name .~ "hello" & #field .~ [def, def & #name .~ "bye"]:: DescriptorProto) {name: "hello" field { } field { name: "bye" }} ``` Also bump the resolver for bootstrapping to match the regular build. Hide message constructors. showsPrec, and bootstrap

They're not that useful for constructing values (especially after adding the "unknown fields" case) and were previously mostly used for documentation. Now that google#172 has landed, we have a better source of documentation that doesn't expose internal details. Also change the `Show` instance for messages to be a wrapper around `showMessageShort`. This both hides the now-inaccessible record fields and shortens the output in most cases. For example: ``` > print (def :: DescriptorProto) {} > print (def & #name .~ "hello" & #field .~ [def, def & #name .~ "bye"]:: DescriptorProto) {name: "hello" field { } field { name: "bye" }} ``` Also bump the resolver for bootstrapping to match the regular build. Hide message constructors. showsPrec, and bootstrap

Finishes #158. They're not that useful for constructing values (especially after adding the "unknown fields" case) and were previously mostly used for documentation. Now that #172 has landed, we have a better source of documentation that doesn't expose internal details. Also change the `Show` instance for messages to be a wrapper around `showMessageShort`. This both hides the now-inaccessible record fields and shortens the output in most cases. For example: ``` > print (def :: DescriptorProto) {} > print (def & #name .~ "hello" & #field .~ [def, def & #name .~ "bye"]:: DescriptorProto) {name: "hello" field { } field { name: "bye" }} ``` Also bump the resolver for bootstrapping to match the regular build.

Add haddock comments which give better documentation for fields lens docs for fields and link them to their long-form variants.

Now that we define our own `Module` type (google#172), we don't need `getModuleName` to return a `Maybe`.

…#174) Finishes google#158. They're not that useful for constructing values (especially after adding the "unknown fields" case) and were previously mostly used for documentation. Now that google#172 has landed, we have a better source of documentation that doesn't expose internal details. Also change the `Show` instance for messages to be a wrapper around `showMessageShort`. This both hides the now-inaccessible record fields and shortens the output in most cases. For example: ``` > print (def :: DescriptorProto) {} > print (def & #name .~ "hello" & #field .~ [def, def & #name .~ "bye"]:: DescriptorProto) {name: "hello" field { } field { name: "bye" }} ``` Also bump the resolver for bootstrapping to match the regular build.

Add haddock comments which give better documentation for fields lens docs for fields and link them to their long-form variants.

…#174) Finishes google#158. They're not that useful for constructing values (especially after adding the "unknown fields" case) and were previously mostly used for documentation. Now that google#172 has landed, we have a better source of documentation that doesn't expose internal details. Also change the `Show` instance for messages to be a wrapper around `showMessageShort`. This both hides the now-inaccessible record fields and shortens the output in most cases. For example: ``` > print (def :: DescriptorProto) {} > print (def & #name .~ "hello" & #field .~ [def, def & #name .~ "bye"]:: DescriptorProto) {name: "hello" field { } field { name: "bye" }} ``` Also bump the resolver for bootstrapping to match the regular build.

judah and others added 6 commits January 11, 2018 12:36

comments, but Haddock rendering is bad

d986da9

Merge branch 'master' into field-comments

521fb08

Simple merge typo.

6972847

Simply uncomment a bunch of existing decls.

67d6e3a

Commit bootstrap.hs' output.

d6d6e10

Give slightly better looking lens docs for fields and link them to th…

321f59c

…eir long-form variants.

judah self-requested a review January 19, 2018 22:41

judah requested changes Jan 22, 2018

View reviewed changes

Fix Judah's comments and remove a warning from the build.

1175585

Cleanup a forgotten version hack.

66b3f57

judah approved these changes Jan 23, 2018

View reviewed changes

judah merged commit 8898a86 into google:master Jan 23, 2018

judah added a commit to judah/proto-lens that referenced this pull request Jan 24, 2018

Simplify an API slightly.

5955380

Now that we define our own `Module` type (google#172), we don't need `getModuleName` to return a `Maybe`.

judah mentioned this pull request Jan 24, 2018

Simplify an API slightly. #173

Merged

judah added a commit that referenced this pull request Jan 24, 2018

Simplify an API slightly. (#173)

fe05638

Now that we define our own `Module` type (#172), we don't need `getModuleName` to return a `Maybe`.

judah mentioned this pull request Jan 30, 2018

Hide the constructors and raw record fields of message types. #174

Merged

avdv pushed a commit to avdv/proto-lens that referenced this pull request Aug 9, 2023

Field comments (google#172)

03717ca

Add haddock comments which give better documentation for fields lens docs for fields and link them to their long-form variants.

avdv pushed a commit to avdv/proto-lens that referenced this pull request Aug 9, 2023

Simplify an API slightly. (google#173)

3823642

Now that we define our own `Module` type (google#172), we don't need `getModuleName` to return a `Maybe`.

ylecornec pushed a commit to ylecornec/proto-lens that referenced this pull request Feb 19, 2024

Field comments (google#172)

011ac8a

Add haddock comments which give better documentation for fields lens docs for fields and link them to their long-form variants.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Field comments #172

Field comments #172

clavoie-takt commented Jan 19, 2018 •

edited by judah

Loading

googlebot commented Jan 19, 2018

clavoie-takt commented Jan 19, 2018

googlebot commented Jan 19, 2018

judah commented Jan 19, 2018

judah left a comment

judah Jan 22, 2018

judah Jan 22, 2018

clavoie-takt commented Jan 22, 2018 via email

Field comments #172

Field comments #172

Conversation

clavoie-takt commented Jan 19, 2018 • edited by judah Loading

googlebot commented Jan 19, 2018

clavoie-takt commented Jan 19, 2018

googlebot commented Jan 19, 2018

judah commented Jan 19, 2018

judah left a comment

Choose a reason for hiding this comment

judah Jan 22, 2018

Choose a reason for hiding this comment

judah Jan 22, 2018

Choose a reason for hiding this comment

clavoie-takt commented Jan 22, 2018 via email

clavoie-takt commented Jan 19, 2018 •

edited by judah

Loading