clickhouse-go icon indicating copy to clipboard operation
clickhouse-go copied to clipboard

NOP/Dummy connection

Open GTB3NW opened this issue 3 years ago • 5 comments

Is your feature request related to a problem? Please describe. Adding nil checks in every location adds verbosity to code

Describe the solution you'd like A dummy or NOP connection to serve the purpose of a drop in replacement for a real connection

Describe alternatives you've considered Currently adding nil checks every time a connection could be used

Additional context N/A

GTB3NW avatar Jun 15 '22 11:06 GTB3NW

@GTB3NW can you provide an example of usage that requires these nil checks? Nil checks area common go pattern but appreciate it might be useful to be able to mock driver in some scenarios.

gingerwizard avatar Jun 17 '22 10:06 gingerwizard

For sure @gingerwizard, it's a fairly idiomatic check in go, however in terms of SDK usage I think a no-op driver would be ideal to reduce nil check bloats (if anything, nil check bloat is one of the main criticisms of the language).

Two examples:

if n.chConn != nil {
		ctx1, cancel1 := context.WithTimeout(context.Background(), 1*time.Second)
		defer cancel1()
		if err := n.chConn.Ping(ctx1); err != nil {
			return err
		}
	}

// more code here

Okay, that's not too bad, but it serves as a basic example, the second example might show where it bloats the code..

	var batch driver.Batch
	var err error
	if n.chConn != nil {
		batch, err = n.chConn.PrepareBatch(context.Background(), MY_CONST_DSL)
		if err != nil {
			return err
		}
	}

      for _, foo := range bar {
          // do some work which can't be behind the nil check, for example stuff which is unit tested

			if n.chConn != nil {
				if err := batch.AppendStruct(&types.MyEvent{
					EventID:       uuid.New(),
				}); err != nil {
					return err
				}
			}
      }

    	if n.chConn != nil {
		if err := batch.Send(); err != nil {
			return err
		}
	}

sorry for the formatting, I've cobbled it together

GTB3NW avatar Jun 17 '22 12:06 GTB3NW

I dont quite understand why you can't just implement the Conn interface with your own dummy connection? it's not the responsibility of the driver to provide dummy test connections.

gingerwizard avatar Jul 12 '22 13:07 gingerwizard

I dont quite understand why you can't just implement the Conn interface with your own dummy connection?

That's an option, honestly, not sure why I didn't think about that

It's not the responsibility of the driver to provide dummy test connections.

I'd disagree but I don't want to come off as a naggy user lol. NOP implementations are really common for situations where you're not looking to test the third party library. I'd argue strongly that the less friction involved for people to adopt your library, the more people will pick it up.

Thanks for getting back to me, please don't take the above as criticism because I do appreciate the library it's great. If I get a moment I will PR a NOP connection with your blessing.

GTB3NW avatar Jul 12 '22 18:07 GTB3NW

@GTB3NW np at all. Happy to accept such a PR if its cleanly seperated e.g. under ./test-lib

gingerwizard avatar Jul 12 '22 19:07 gingerwizard