Update Reader Value APIs to symbol token abstraction #158
Conversation
This reverts commit 698d352.
…b.com:byronlin-bq/ion-go into Update_value_apis_to_symbol_token_abstraction
byronlin13
changed the title
cmd/ion-go/process.go
Outdated
| name, e := in.FieldName() | ||
| if e != nil { | ||
| return p.error(read, err) | ||
| } | ||
| if name != nil { |
There was a problem hiding this comment.
if name != nil && name.Text != nil { ?
| @@ -38,11 +38,11 @@ func TestWriteBinaryStruct(t *testing.T) { | |||
| w.BeginStruct() | |||
| w.EndStruct() | |||
|
|
|||
| w.Annotation("foo") | |||
| w.Annotation(SymbolToken{Text: newString("foo"), LocalSID: SymbolIDUnknown}) | |||
There was a problem hiding this comment.
Suggestion:
func getSymbolToken(text string, sid ...int) {
if len(sid) > 0 {
return SymbolToken{Text: &text, LocalSID: sid)
}
return SymbolToken{Text: &text, LocalSID: SymbolIDUnknown)
}
....
w.Annotation(getSymbolToken("foo"))
And then use the same in other exampls
There was a problem hiding this comment.
I rather not do this in the test because I want the symboltoken to explicitly state the text SID so it is clear to the person reading this test
| @@ -211,7 +246,7 @@ func containersEquality(this, other interface{}) bool { | |||
| default: | |||
| otherItem := other.(ionItem) | |||
| thisItem := this.(ionItem) | |||
| if thisItem.fieldName == otherItem.fieldName && thisItem.equal(otherItem) { | |||
| if thisItem.fieldName.Equal(&otherItem.fieldName) && thisItem.equal(otherItem) { | |||
There was a problem hiding this comment.
&otherItem.fieldName wouldn't it be better to compare the values?
There was a problem hiding this comment.
Currently equal() function takes in a non-pointer
func (i *ionItem) equal(o ionItem) bool {
If I were to do your suggestion it would be:
default:
otherItem := other.(*ionItem)
thisItem := this.(*ionItem)
if thisItem.fieldName.Equal(&otherItem.fieldName) && thisItem.equal(*otherItem) {
return true
}
but it is failing right now because the value APIs arent a pointer yet
| w.Annotation("foo") | ||
| w.Annotation("$bar") | ||
| w.Annotation(".baz") | ||
| w.Annotation(SymbolToken{Text: newString("foo"), LocalSID: SymbolIDUnknown}) |
There was a problem hiding this comment.
Up to you, but if you think it makes it more readable you can use helper method for these like the suggestion on binarywriter_test
ion/integration_test.go
Outdated
| for _, a := range an { | ||
| if a == "embedded_documents" { | ||
| if a.Text != nil && *a.Text == "embedded_documents" { | ||
| return true |
There was a problem hiding this comment.
IIRC, doesn't this need to be the first annotation? (Rather than anywhere in the annotations list?)
There was a problem hiding this comment.
The ion test data files have 'embedded_documents' annotation by itself, it makes sense that it should be the first annotation, updated
Issue #, if available:
#151
Description of changes:
-Update Reader API to return SymbolToken abstraction instead of string representation of Symbols.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.