haze518 commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2173265154
##########
foreign/go/samples/consumer/consumer.go:
##########
@@ -38,39 +38,32 @@ const (
)
func main() {
- factory := &IggyClientFactory{}
- config := IggyConfiguration{
- BaseAddress: "127.0.0.1:8090",
- Protocol: Tcp,
- }
-
- messageStream, err := factory.CreateMessageStream(config)
+ cli, err :=
iggycli.NewIggyClientBuilder().WithTcp().WithServerAddress("127.0.0.1:8090").Build()
if err != nil {
panic(err)
}
-
- _, err = messageStream.LogIn(LogInRequest{
+ _, err = cli.LoginUser(LoginUserRequest{
Username: "iggy",
Password: "iggy",
})
if err != nil {
panic("COULD NOT LOG IN")
}
- if err = EnsureInsfrastructureIsInitialized(messageStream); err != nil {
+ if err = EnsureInsfrastructureIsInitialized(cli); err != nil {
panic(err)
}
- if err := ConsumeMessages(messageStream); err != nil {
+ if err := ConsumeMessages(*cli); err != nil {
Review Comment:
here you don’t need to dereference, since ConsumeMessages expects an
interface, so you can just pass a pointer
##########
foreign/go/iggycli/iggy_client.go:
##########
@@ -0,0 +1,193 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iggycli
+
+import (
+ . "github.com/apache/iggy/foreign/go/contracts"
+ ierror "github.com/apache/iggy/foreign/go/errors"
+)
+
+type IggyClient struct {
Review Comment:
I don’t quite get it — why do we need this wrapper around the client? It
seems like it doesn’t really do much, except just forward the call to the
client’s method
##########
foreign/go/iggycli/client_builder.go:
##########
@@ -0,0 +1,93 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iggycli
+
+import (
+ "context"
+ "fmt"
+
+ ierror "github.com/apache/iggy/foreign/go/errors"
+ "github.com/apache/iggy/foreign/go/tcp"
+)
+
+type IggyClientBuilder struct {
+ client Client
+}
+
+// NewIggyClientBuilder create a new IggyClientBuilder
+// This is not enough to build the IggyClient instance. You need to provide
the client configuration or the client
+// implementation for the specific transport.
+func NewIggyClientBuilder() *IggyClientBuilder {
Review Comment:
What do you think about this approach (yeah, it’s me with my options again
😄)?
If we created an enum to determine which transport the user wants to use,
and also added With... options for each transport’s config, we’d end up with
something like this:
```go
// client_builder.go
type Transport int
const (
TransportTCP Transport = iota
)
type Config struct {
transport Transport
tcpConfig *tcp.ClientConfig
}
func NewDefaultConfig() *Config {
return &Config{
transport: TransportTCP,
tcpConfig: NewDefaultTCPConfig(),
}
}
type Option func(*Config)
func WithTCP(tcpOpts ...TCPOption) Option {
return func(c *Config) {
c.transport = TransportTCP
tcp := NewDefaultTCPConfig()
for _, o := range tcpOpts {
o(tcp)
}
c.tcpConfig = tcp
}
}
func New(options ...Option) (Client, error) {
cfg := NewDefaultConfig()
for _, o := range options {
o(cfg)
}
switch cfg.transport {
case TransportTCP:
cli, err := tcp.NewIggyTcpClient(*cfg.tcpConfig)
if err != nil {
return nil, err
}
return cli, nil
default:
return nil, fmt.Errorf("unknown transport type: %d",
cfg.transport)
}
}
// tcp_core.go
type ClientConfig struct {
Ctx context.Context
ServerAddress string
HeartbeatInterval time.Duration
}
type TCPOption func(*ClientConfig)
func WithServerAddress(address string) TCPOption {
return func(c *ClientConfig) {
c.ServerAddress = address
}
}
```
Honestly, I just like this approach better — compared to a builder, there’s
way less boilerplate (no need for extra chains or factories), and it feels much
easier to expand in the future. Also, it’s more idiomatic for go, at least in
my opinion (even if that’s up for debate).
But I’d be interested to hear your thoughts on this
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
