Implement Decoder and Encoder #5
Conversation
SVilgelm
force-pushed
the
add-encoder-decoder
branch
3 times, most recently
from
d148579
to
06029ce
Compare
|
I have also raised a PR with similar code to |
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.
Would be nice to have tests for this method to compare expected output.
There was a problem hiding this comment.
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.
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.
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 |
SVilgelm
force-pushed
the
add-encoder-decoder
branch
from
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 |
| 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.
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.
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.
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.
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.
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.:
unmarshalEqualdecodeEqualdecodeEqualKnownFields
|
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