[GitHub] [thrift] fishy commented on a change in pull request #2296: THRIFT-5322: Implement TConfiguration in Go library

[GitBox <gi...@apache.org>]

fishy commented on a change in pull request #2296:
URL: https://github.com/apache/thrift/pull/2296#discussion_r544740347



##########
File path: lib/go/thrift/header_protocol.go
##########
@@ -45,65 +47,49 @@ type THeaderProtocol struct {
 // so the underlying transport should be a raw socket transports (TSocket or TSSLSocket),
 // instead of rich transports like TZlibTransport or TFramedTransport.
 func NewTHeaderProtocol(trans TTransport) *THeaderProtocol {
-	p, err := newTHeaderProtocolWithProtocolID(trans, THeaderProtocolDefault)
-	if err != nil {
-		// Since we used THeaderProtocolDefault this should never happen,
-		// but put a sanity check here just in case.
-		panic(err)
-	}
-	return p
+	return newTHeaderProtocolConf(trans, nil)
 }
 
-func newTHeaderProtocolWithProtocolID(trans TTransport, protoID THeaderProtocolID) (*THeaderProtocol, error) {
-	t, err := NewTHeaderTransportWithProtocolID(trans, protoID)
-	if err != nil {
-		return nil, err
-	}
-	p, err := t.protocolID.GetProtocol(t)
-	if err != nil {
-		return nil, err
-	}
+func newTHeaderProtocolConf(trans TTransport, cfg *TConfiguration) *THeaderProtocol {
+	t := NewTHeaderTransportConf(trans, cfg)
+	p, _ := t.cfg.GetTHeaderProtocolID().GetProtocol(t)
+	PropagateTConfiguration(p, cfg)
 	return &THeaderProtocol{
 		transport: t,
 		protocol:  p,
-	}, nil
+		cfg:       cfg,
+	}
 }
 
 type tHeaderProtocolFactory struct {
-	protoID THeaderProtocolID
+	cfg *TConfiguration
 }
 
 func (f tHeaderProtocolFactory) GetProtocol(trans TTransport) TProtocol {
-	p, err := newTHeaderProtocolWithProtocolID(trans, f.protoID)
-	if err != nil {
-		// Currently there's no way for external users to construct a
-		// valid factory with invalid protoID, so this should never
-		// happen. But put a sanity check here just in case in the
-		// future a bug made that possible.
-		panic(err)
-	}
-	return p
+	return newTHeaderProtocolConf(trans, f.cfg)
 }
 
-// NewTHeaderProtocolFactory creates a factory for THeader with default protocol
-// ID.
-//
-// It's a wrapper for NewTHeaderProtocol
+func (f *tHeaderProtocolFactory) SetTConfiguration(cfg *TConfiguration) {
+	f.cfg = cfg
+}
+
+// NewTHeaderProtocolFactory creates a factory for THeader with default
+// TConfiguration.
 func NewTHeaderProtocolFactory() TProtocolFactory {
-	return tHeaderProtocolFactory{
-		protoID: THeaderProtocolDefault,
-	}
+	return NewTHeaderProtocolFactoryConf(nil)
 }
 
-// NewTHeaderProtocolFactoryWithProtocolID creates a factory for THeader with
-// given protocol ID.
-func NewTHeaderProtocolFactoryWithProtocolID(protoID THeaderProtocolID) (TProtocolFactory, error) {

Review comment:
       note that this is a breaking change from a previous version on master branch. however it is not a breaking change from 0.13.0.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org