Merge of fernomac/ion-go #26

therapon · 2020-05-08T04:16:55Z

Description of changes:

Brings in all the code from fernomac/ion-go

Removes existing code (backed up on branch original-repo-bkp)
Merge's fernomac/ion-go and keeps history from both repos. Fixes the following conflicts
- LICENSE - were almost identical, some extra lines and an ending sentence
- README - did my best as a first pass.
- NOTICE - kept ours
Moved the code under ion folder
- updated path in tests for ion-test
Updated ion-tests to latest commit
- updated blacklist
- filtered files with suffix md
Added empty internal folder for private packages

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the Apache 2.0 license.

Add token after adding it to repo's secrets

codecov-io · 2020-05-08T07:44:59Z

Codecov Report

Merging #26 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #26   +/-   ##
=======================================
  Coverage   71.81%   71.81%           
=======================================
  Files          22       22           
  Lines        4528     4528           
=======================================
  Hits         3252     3252           
  Misses        791      791           
  Partials      485      485

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cddde4...0cddde4. Read the comment docs.

fernomac

Code looks good to me, I'm excited to have this officially maintained!

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

I would, however, like to explicitly opt out of this bit. You are welcome to use, modify, etc my parts of this "contribution" under the terms of the Apache license I applied to it, and only under those terms.

Specifically, section 4D of that license asks you to include the Copyright 2019 David Murray from my NOTICE file in any redistributions. Standard practice and my preference would be for you to include that line in your NOTICE file, alongside the copyright notice for any modifications made by Amazon.

Fix that up and I'm happy to approve!

README.md

therapon · 2020-05-08T17:58:50Z

Code looks good to me, I'm excited to have this officially maintained!

Great.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

I would, however, like to explicitly opt out of this bit. You are welcome to use, modify, etc my parts of this "contribution" under the terms of the Apache license I applied to it, and only under those terms.

Sorry that is the template that I skip for the common case of a PR. Updated to read:

"By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the Apache 2.0 license."

Specifically, section 4D of that license asks you to include the Copyright 2019 David Murray from my NOTICE file in any redistributions. Standard practice and my preference would be for you to include that line in your NOTICE file, alongside the copyright notice for any modifications made by Amazon.

Fix that up and I'm happy to approve!

You are right, since there was a NOTICE file to begin with. Updated.

fernomac

Awesome, thanks!

fernomac · 2020-05-11T15:06:59Z

Had a bit of free time yesterday and added support for pretty-printing and a first draft of an ion-go CLI tool. Once this is merged I'll rebase and send them over here as PRs.

almann

Overall this seems reasonable based on what we discussed and what I could see (I looked mostly at your commits).

I was unable to comment on the file, but do we need internal/.keep since we seem to be eliminating that module.

therapon · 2020-05-12T21:58:33Z

I was unable to comment on the file, but do we need internal/.keep since we seem to be eliminating that module.

During an offline discussion there was a mention for creating a private package. I seeded the folder as a convenience. I can take it off now and we can add it later if needed.

fernomac · 2020-05-13T00:57:46Z

FWIW I did end up sticking a small bit of code in internal/ in the aforementioned CLI tool branch. Would be fine to expose from ion/ if you wanted to though.

mrutter-amzn

Nothing that seems like it should block the merge. Several things to cut issues about to discuss.

README.md

mrutter-amzn · 2020-05-12T16:32:11Z

README.md

+  if err != nil {
+    panic(err)
+  }
+  fmt.Printf("--- t:\n%v\n\n", t)


These Printfs should be followed with a comment that shows what would be printed. The code can't be copy/pasted into a playground because it relies on an external library.

README.md

mrutter-amzn · 2020-05-15T18:57:49Z

ion/reader.go

+	// Next advances the Reader to the next position in the current value stream.
+	// It returns true if this is the position of an Ion value, and false if it
+	// is not. On error, it returns false and sets Err.
+	Next() bool


It would be slightly more idiomatic and user-friendly to return the Type from Next() and have one of the possible values indicate error.

I was following the pattern of sql.Rows, and personally find that style to be friendlier than (what I imagine to be) the alternative?

for r.Next() { switch r.Type() { ... } } if r.Err() { return r.Err() }

vs

for { t := r.Next() if t == ion.ErrorType { return r.Err() } if t == ion.EndOfSequenceType { break } switch t { ... } }

(Unless I'm misunderstanding what you're saying, which is entirely possible. :))

I was thinking more like:

for { switch r.Next() { case ion.ErrorType: return r.Err() case ion.EndOfSequenceType: return nil case ... } }

Which if you think about it could be simplified to

for { switch r.Next() { case ion.ErrorType, ion.EndOfSequenceType: return r.Err() case ... } }

Ah. That's better than my attempt, although I still prefer things the sql.Rows way. Not my problem any more so feel free to change it if you disagree. :)

I made the suggestion because it is similar to text.Scanner and I was thinking that the Reader is kind of like an ion.Scanner. I don't think that me saying my suggestion was more idiomatic was fair. They are both idiomatic approaches.

Created #28

I assume Type should be mapping to Ion Types.
In case of EndOfSequenceType or ErrorType could we return NoType ?
When I check ion-dotnet, I see we have similar logic for MoveNext() in BinaryReader and TextReader.

mrutter-amzn · 2020-05-15T20:03:34Z

ion/unmarshal.go

+// UnmarshalStr unmarshals Ion data from a string to the given object.
+func UnmarshalStr(data string, v interface{}) error {


Not really necessary and adds another function to the Unmarshal story.

mrutter-amzn · 2020-05-15T20:10:46Z

ion/unmarshal.go

+// A Decoder decodes go values from an Ion reader.
+type Decoder struct {
+	r Reader
+}
+
+// NewDecoder creates a new decoder.
+func NewDecoder(r Reader) *Decoder {
+	return &Decoder{
+		r: r,
+	}
+}
+
+// NewTextDecoder creates a new text decoder. Well, a decoder that uses a reader with
+// no shared symbol tables, it'll work to read binary too if the binary doesn't reference
+// any shared symbol tables.
+func NewTextDecoder(in io.Reader) *Decoder {


I don't think that the decoder story is clear. What if I have binary data that is composed of multiple Ion datagrams each one should decode into an instance of a struct? What is the binary story? If the reader is supposed to be able to determine text or binary, then why are there multiple ways to create a Decoder?

Decoders read one value at a time, without respect to datagrams (the symbol table gets reset under the covers when we hit a new BVM, but that isn't exposed up through the Reader). I didn't have a use case for decoding an entire datagram at a time, if you do please feel free to refactor to support that.

NewTextDecoder is a convenience since I was mostly decoding text Ion and got sick of typing NewDecoder(NewReader(in)). :) It's not well-named, feel free to change or delete it.

The Encode/Decode as well as the Marshall/Unmarshall are a good seed and is already on our todo list. #30

mrutter-amzn · 2020-05-15T20:13:07Z

ion/writer.go

+	// FieldName sets the field name for the next value written.
+	FieldName(val string) error


SetFieldName or StartField?

mrutter-amzn · 2020-05-15T20:14:08Z

ion/writer.go

+	// Finish finishes writing values and flushes any buffered data.
+	Finish() error


It seems like the expectation is that there would be no more writes after a call to Finish. If that is the case, then Close seems like a better option.

It's the direct equivalent of the finish method from ion-java; when writing binary Ion without a pre-canned local symbol table, you might want to Finish periodically to avoid buffering the whole output in memory.

I'm wondering if Flush() error would be a more idiomatic alternative then.

therapon

It looks to me that merging this as a seed is ok with the majority of reviewers. I added some minor fixes and created issues for comments made.

Unless I hear otherwise, I plan to merge this PR by Tuesday (05/19).

fernomac added 30 commits June 10, 2019 17:10

basic text writer implementation

1750293

go fmt

7ac7e33

parsing decimals and some math

f8208a5

more decimal bikeshedding

797d9a9

refactorizing to avoid circular references

46c843f

fine, go fmt

b5b2778

symboltable, reader interface

303e8aa

start of tokenizer

7b32f6c

minor fixups

dcb9d96

Initial textreader impl

f1d6578

handling for special symbols

b5f7d6b

strings and integers

e4a4ba2

floats, doubles, timestamps

e75397a

blobs and clobs

ba3adeb

structs and lists

8f8d5dc

sexps

66de8b9

Adding test data submodule

c11d8db

standard test suite for text reader, bugfixes

a69cbd3

test refactoring, bugfixes

df048f4

Truncate text timestamps with more than nanosecond precision

1991b65

marshal

7532672

make marshaller public

8400307

Writer.WriteValue

e35a521

marshaller->encoder

9c1a186

unmarshal

d5ef309

tackling some todos

22e024c

binary writer

7501e3a

stuff and stuff

a9ee27b

refactoring binary writer

c5e0d4c

changing some things around here

cb4e1ba

therapon requested review from mrutter-amzn and pbcornell May 8, 2020 04:17

therapon marked this pull request as ready for review May 8, 2020 04:18

therapon added 4 commits May 8, 2020 00:17

Trying code coverage

678efdf

Adding codecov token

16d77f3

Add token after adding it to repo's secrets

Kick the build again

3cc0061

Use latest codecov action version

b1bb777

fernomac suggested changes May 8, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update NOTICE per Apache v2

2dff52b

fernomac previously approved these changes May 8, 2020

View reviewed changes

Adding PR template

9ae6004

therapon dismissed fernomac’s stale review via 9ae6004 May 8, 2020 20:15

almann previously approved these changes May 12, 2020

View reviewed changes

mrutter-amzn previously approved these changes May 15, 2020

View reviewed changes

therapon dismissed stale reviews from mrutter-amzn and almann via 749c0a7 May 17, 2020 05:39

fix go import in README

749c0a7

Address PR comments

0cddde4

therapon commented May 17, 2020

View reviewed changes

therapon merged commit e32dee6 into amazon-ion:master May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge of fernomac/ion-go #26

Merge of fernomac/ion-go #26

therapon commented May 8, 2020 •

edited

Loading

codecov-io commented May 8, 2020 •

edited

Loading

fernomac left a comment

therapon commented May 8, 2020

fernomac left a comment

fernomac commented May 11, 2020

almann left a comment

therapon commented May 12, 2020

fernomac commented May 13, 2020

mrutter-amzn left a comment

mrutter-amzn May 12, 2020

mrutter-amzn May 15, 2020

fernomac May 15, 2020

mrutter-amzn May 15, 2020

mrutter-amzn May 15, 2020

fernomac May 15, 2020

mrutter-amzn May 15, 2020

therapon May 17, 2020

R-maan May 21, 2020

mrutter-amzn May 15, 2020

therapon May 17, 2020

mrutter-amzn May 15, 2020

fernomac May 15, 2020

therapon May 17, 2020 •

edited

Loading

mrutter-amzn May 15, 2020

mrutter-amzn May 15, 2020

fernomac May 15, 2020

mrutter-amzn May 15, 2020

therapon May 17, 2020

therapon left a comment

		// UnmarshalStr unmarshals Ion data from a string to the given object.
		func UnmarshalStr(data string, v interface{}) error {

		// FieldName sets the field name for the next value written.
		FieldName(val string) error

		// Finish finishes writing values and flushes any buffered data.
		Finish() error

Merge of fernomac/ion-go #26

Merge of fernomac/ion-go #26

Conversation

therapon commented May 8, 2020 • edited Loading

codecov-io commented May 8, 2020 • edited Loading

Codecov Report

fernomac left a comment

Choose a reason for hiding this comment

therapon commented May 8, 2020

fernomac left a comment

Choose a reason for hiding this comment

fernomac commented May 11, 2020

almann left a comment

Choose a reason for hiding this comment

therapon commented May 12, 2020

fernomac commented May 13, 2020

mrutter-amzn 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

therapon May 17, 2020 • edited Loading

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

therapon left a comment

Choose a reason for hiding this comment

therapon commented May 8, 2020 •

edited

Loading

codecov-io commented May 8, 2020 •

edited

Loading

therapon May 17, 2020 •

edited

Loading