Skip to content

Conversation

mikegunter
Copy link

…istribution

The Unicode escaping that the Protocol Buffer distribution supports is
not implemented for now.

Add tests.

Mike Gunter added 2 commits September 14, 2016 06:53
…ping as Protobuf distribution

The Unicode escaping that the Protocol Buffer distribution supports is
not implemented for now.

Add tests.
-- so you can't use Prelude.read to parse the strings created here.
pprintByteString :: String -> Data.ByteString.ByteString -> Doc
pprintByteString name x = text name <> colon <+> char '\"'
<> text (concatMap escape $ Data.ByteString.unpack x) <> char '\"'
Copy link
Contributor

@judah judah Sep 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include the other ascii escapes like '\a', '\b', etc. in our TextFormat output?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the distribution, see https://github.com/google/protobuf/blob/master/src/google/protobuf/stubs/strutil.cc#L578, in not emitting those.

This inspired me to add a test for parsing them, though.

Copy link
Contributor

@judah judah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good!

@mikegunter
Copy link
Author

See the contents of escapeMessage and escapeRendered.

-m

On Wed, Sep 14, 2016 at 9:34 PM, Judah Jacobson [email protected]
wrote:

@judah commented on this pull request.

In proto-lens-tests/tests/text_format_test.hs
#48 (review):

@@ -57,8 +65,40 @@ main = testMain
, testCase "Render multiple lines" $
"d: 1\nd: 2\nd: 3" @=?
showMessageWithLineLength 3 (def4 & d .~ [1, 2, 3])

  • , readFrom
  •     "Parse string with escape sequences"
    
  •      -- '\o172' == '\x7a' == 'z'
    
  •     (Just $ def2 & b .~ "\o1\o12\o123\x2\o172z3z3")
    
  •     (Data.Text.Lazy.pack "b: \"\001\012\123\002\172\x7a3\1723\"")
    
  • , testCase "Render string with escape sequences" $
  •    escapeRendered @=? showMessageShort escapeMessage
    
  • , readFrom "Parse rendered string with escape sequences"
  •           (Just escapeMessage) (Data.Text.Lazy.pack escapeRendered)
    
  • , testCase "Render bytes" $
  •     invalidUTF8BytesRendered @=? showMessage invalidUTF8BytesMessage
    
  • , readFrom "Parse single-quote-delimited string"

Can you add a test for an escape quote nested inside a string (assuming we
don't already have it)? For example, "abc"def"


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#48 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AQ9hHHHmVXSpqFvVzj1w-cY8-Q6iXtRsks5qqMrXgaJpZM4J8zbq
.

@judah judah merged commit 093d3a5 into google:master Sep 15, 2016
judah added a commit to judah/proto-lens that referenced this pull request Jan 3, 2017
Also split out text_format_test's .proto file rather than sharing it
with canonical_test.

Note: PR google#48 previously referenced this bug, but it looks like that was a
typo since that fix was for string escaping, not field ordering.
judah added a commit to judah/proto-lens that referenced this pull request Jan 3, 2017
Also split out text_format_test's .proto file rather than sharing it
with canonical_test.

Also changed the "field name" stored in Data.ProtoLens.Message.FieldDescriptor
to be the name of that type, which is more consistent with how TextFormat
prints/parses such messages.

Note: PR google#48 previously referenced this bug, but it looks like that was a
typo since that fix was for string escaping, not field ordering.
judah added a commit to judah/proto-lens that referenced this pull request Jan 3, 2017
Also split out text_format_test's .proto file rather than sharing it
with canonical_test.

Also changed the "field name" stored in Data.ProtoLens.Message.FieldDescriptor
to be the name of that type, which is more consistent with how TextFormat
prints/parses such messages.  This change simplifies the new code.

Note: PR google#48 previously referenced this bug, but it looks like that was a
typo since that fix was for string escaping, not field ordering.
judah added a commit that referenced this pull request Jan 4, 2017
Also split out text_format_test's .proto file rather than sharing it
with canonical_test.

Also changed the "field name" stored in Data.ProtoLens.Message.FieldDescriptor
to be the name of that type, which is more consistent with how TextFormat
prints/parses such messages.  This change simplifies the new code.

Note: PR #48 previously referenced this bug, but it looks like that was a
typo since that fix was for string escaping, not field ordering.
avdv pushed a commit to avdv/proto-lens that referenced this pull request Aug 9, 2023
Also split out text_format_test's .proto file rather than sharing it
with canonical_test.

Also changed the "field name" stored in Data.ProtoLens.Message.FieldDescriptor
to be the name of that type, which is more consistent with how TextFormat
prints/parses such messages.  This change simplifies the new code.

Note: PR google#48 previously referenced this bug, but it looks like that was a
typo since that fix was for string escaping, not field ordering.
ylecornec pushed a commit to ylecornec/proto-lens that referenced this pull request Feb 19, 2024
…ping as Protobuf d… (google#48)

* Fix Issues google#20, google#40, and google#45 by using the same escaping as Protobuf distribution

The Unicode escaping that the Protocol Buffer distribution supports is
not implemented for now.

Add tests.
ylecornec pushed a commit to ylecornec/proto-lens that referenced this pull request Feb 19, 2024
Also split out text_format_test's .proto file rather than sharing it
with canonical_test.

Also changed the "field name" stored in Data.ProtoLens.Message.FieldDescriptor
to be the name of that type, which is more consistent with how TextFormat
prints/parses such messages.  This change simplifies the new code.

Note: PR google#48 previously referenced this bug, but it looks like that was a
typo since that fix was for string escaping, not field ordering.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants