-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Decoder and Encoder #5
Conversation
d148579
to
06029ce
Compare
I have also raised a PR with similar code to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the look of where this is going! Thanks for submitting! We've already started using the Decoder and Encoder methods, so perhaps it would be easier to enabled wrappers around those? I see a few copy and pasted comments, which suggests to me "refactor".
yaml.go
Outdated
|
||
// KnownFields ensures that the keys in decoded mappings to | ||
// exist as fields in the struct being decoded into. | ||
func (dec *Decoder) KnownFields() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have tests for this method to compare expected output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I will add the test for this method
yaml.go
Outdated
|
||
// DisallowUnknownFields configures the JSON decoder to error out if unknown | ||
// fields come along, instead of dropping them by default. | ||
func DisallowUnknownFields(d *json.Decoder) *json.Decoder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing tests to understand the use-case. Also, is there are reason why this method is not attached to the main Decoder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to have it as a separate JSONOpt, it is in original ghodss code and we use it a lot, but on another hand it should be always used by calling KnownFields method. will refactor
as I said I just copied my PR I raised for sigs.k8s.io fork and did not fully adapt it to your code, let me see how I can make it better |
06029ce
to
141697a
Compare
@samlown Hi, I have refactored Encoder/Decoder to reuse the existent code, extend the tests for testing Unmarshal/Decode and Marshal/Encode functions with strict and non strict mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I'd just check if Close
is required in the Encoders, and double check the io.EOF
issue.
if err != nil { | ||
return nil, fmt.Errorf("error marshaling into JSON: %v", err) | ||
var buf bytes.Buffer | ||
err := NewEncoder(&buf).Encode(o) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this encoder need closing?
@@ -41,20 +77,57 @@ type JSONOpt func(*json.Decoder) *json.Decoder | |||
// Unmarshal converts YAML to JSON then uses JSON to unmarshal into an object, | |||
// optionally configuring the behavior of the JSON unmarshal. | |||
func Unmarshal(y []byte, o interface{}, opts ...JSONOpt) error { | |||
dec := yaml.NewDecoder(bytes.NewReader(y)) | |||
return unmarshal(dec, o, opts) | |||
return NewDecoder(bytes.NewReader(y), opts...).Decode(o) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just discovered a bug around the decoder that may need handling: go-yaml/yaml#639
Basically, if the data is empty or even {}
the Decode
method will return an io.EOF
error. This is not the expected behaviour of the Unmarshal
method.
My PR to fix this is: #7
} | ||
|
||
func unmarshal(dec *yaml.Decoder, o interface{}, opts []JSONOpt) error { | ||
vo := reflect.ValueOf(o) | ||
j, err := yamlToJSON(dec, &vo) | ||
if err != nil { | ||
return fmt.Errorf("error converting YAML to JSON: %v", err) | ||
return fmt.Errorf("error converting YAML to JSON: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one finding the missing %w
! I hadn't noticed that.
} | ||
return nil | ||
} | ||
|
||
// JSONToYAML converts JSON to YAML. | ||
func JSONToYAML(j []byte) ([]byte, error) { | ||
var buf bytes.Buffer | ||
err := jsonToYAML(yaml.NewEncoder(&buf), bytes.NewReader(j)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, the encoder might need closing.
} | ||
return r | ||
} | ||
func unmarshalEqual(t *testing.T, y []byte, s, e interface{}, knowFields bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be knownFields
. I'm not a fan of boolean flags though, they're hard to read. Might it be better to define specific functions for each use-case? i.e.:
unmarshalEqual
decodeEqual
decodeEqualKnownFields
thank for reviewing, I agree that we need to close the decoder even if it is not required, give me a day or tow and I will fix it |
add support for streaming encoding and decoding
closes #4