Skip to content

add nop store#126

Merged
philippgille merged 4 commits intophilippgille:masterfrom
peczenyj:add-nop-store
Oct 31, 2023
Merged

add nop store#126
philippgille merged 4 commits intophilippgille:masterfrom
peczenyj:add-nop-store

Conversation

@peczenyj
Copy link
Copy Markdown
Contributor

Hello

In some scenarios it is interesting add an object that does nothing (for testing, debug, etc)

for instance, imagine I may use one of few different implementations of govk.Store and I decide it based on configuration using some strategy pattern. To test the application without the govk.Store I had two options:

  1. use a nil store and add several if store != nil {... } around the code, or
  2. I had to implement a nop interface by myself

not anymore!

This also perform the validation of key / value like a regular gokv.Store does, to avoid break compatibility.

If you want, I can add an option to disable this validation

Enjoy

@philippgille
Copy link
Copy Markdown
Owner

Hi, thanks a lot! I'll have a look on Tuesday.

Copy link
Copy Markdown
Owner

@philippgille philippgille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your contribution!

Two comments, but not blockers.

Comment thread nop/nop_test.go
t.Helper()

if err == nil {
t.Error("expect error, got nil")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but checking all other *_test.go files in this repo, they use either "Expected an error" or "An error was expected". The exact phrasing doesn't matter, but I'd suggest uppercase and past tense for consistency, e.g.

Suggested change
t.Error("expect error, got nil")
t.Error("Expected error, got nil")

Errors are usually lowercase as they can be chained in a log message (e.g. "couldn't x: failed to y: ...", but here it's a log message and not an error that might be chained, where I like the differentiation.

Again, totally not a blocker though.

Comment thread nop/docs.go Outdated
Signed-off-by: Tiago Peczenyj <tpeczenyj@weborama.com>
Copy link
Copy Markdown
Owner

@philippgille philippgille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change 💪

@philippgille philippgille merged commit 672740b into philippgille:master Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants