Add encoder/decoder interface to configure encoding/decoding #47
Conversation
|
To be backwards compatible, I added back the old functions. Removing them would be a breaking change as they are exported (capital letter) and someone might be using them directly. If you have any opinions on how to do this in a cleaner way, just let me know. I myself am not completely satisfied with this right now. |
|
Any comments @VojtechVitek ? |
There was a problem hiding this comment.
Good job on the PR.
However, there are still some design choices (as suggested in #40) that don't feel right when looking at the clean code diff.
Can we simplify this further?
| // a value `v`. | ||
| type Decoder interface { | ||
| // Decodes a given reader into an interface | ||
| Decode(r io.Reader, req *http.Request, v interface{}) error |
There was a problem hiding this comment.
I think I'll need to revisit the discussion at #40.
I see a r io.Reader -- but req.Body is also an io.Reader. Do we need both?
There was a problem hiding this comment.
Oh well that's right r will be req.Body. But see the comment below for the discussion of the function signature.
| // our default implementations. By setting render.Decode{JSON,XML,Form} you can | ||
| // customize Decoding (e.g. you might want to configure the JSON-decoder) | ||
| var ( | ||
| DecoderJSON Decoder = DecodeJSONInter{} |
There was a problem hiding this comment.
What does Inter stand for?
There was a problem hiding this comment.
Well as I needed to keep the DecodeJSON function (see below), I needed to call the struct which implements the interface differently and decided to go with DecodeJSONInter to signal that this is the version using/conforming to the interface.
Maybe Impl or so would be the more typical choice.
| // XML marshals 'v' to XML, setting the Content-Type as application/xml. It | ||
| // will automatically prepend a generic XML header (see encoding/xml.Header) if | ||
| // one is not found in the first 100 bytes of 'v'. | ||
| // | ||
| // Deprecated: EncoderXML.Encode() should be used. |
There was a problem hiding this comment.
I don't think we want to "deprecate" these functions.
I thought we would just let users overwrite them with a custom implementation.
There was a problem hiding this comment.
Well this issue is linked with your next annotation. Because we wanted to add the Request object, the Interface of the function changes and there was no way of keeping the old function and the new function, with the old function calling simply redirecting to the new one (the request object has to come from somewhere).
So in the end it's all about changing the function signature (deprecating and in a future breaking change remove the old functions) or not. Personally I'd be satisfied with the current signature as well, right now I don't have a use-case where I need the Request object in the en-/decoder implementation.
| err = DecoderJSON.Decode(r.Body, r, v) | ||
| case ContentTypeXML: | ||
| err = DecodeXML(r.Body, v) | ||
| err = DecoderXML.Decode(r.Body, r, v) | ||
| case ContentTypeForm: | ||
| err = DecodeForm(r.Body, v) | ||
| err = DecoderForm.Decode(r.Body, r, v) |
There was a problem hiding this comment.
Looking at this change, I think I was wrong to suggest adding a request object to this func signature.
|
As the discussion of the function signatures is a bigger thing and I find larger discussions within a review rather hard to keep track of, can we please continue the discussion here in the "main thread"? |
|
So are we back at type Decoder interface {
Decode(w io.Reader, v any) (error)
}
type Encoder interface {
Encode(w io.Writer, v any) (error)
}? (this way we also can stick to the current functions which then simply call the encode/decode functions of the encoder/decoder being set) |
|
@VojtechVitek Any thoughts on this basic interface? |
Like discussed in #40 . There are still things to do:
Decoderas name for the interface in thedecoder.gofile where actuallyDecodeis the core component (withEncoderthis is much better as there the termresponderis being used)On the second point: As
DecodeJSONetc were public functions, to be fully backwards compatible, they need to be preserved, right? So maybe the best shot is to keep those functions and in the default implementation ofDecoder/Encoderfor e.g.JSONjust delegate the calls to these old functions, right?As this is not finished yet, it's just a draft PR. Nevertheless I already stated TODOs so if you already have any comment on the current state, don't hesitate.