Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ensure get returns a nil and key not found as per spec #844

Merged
merged 1 commit into from Oct 11, 2021

Conversation

ripienaar
Copy link
Contributor

Signed-off-by: R.I.Pienaar rip@devco.net

@@ -333,6 +333,18 @@ func keyValid(key string) bool {

// Get returns the latest value for the key.
func (kv *kvs) Get(key string) (KeyValueEntry, error) {
e, err := kv.get(key)
if err == ErrKeyDeleted {
return nil, ErrKeyNotFound
Copy link
Contributor Author

@ripienaar ripienaar Oct 11, 2021

Choose a reason for hiding this comment

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

It's critical that from the perspective of a user of Get() its always found/not found when delete or purge operation, we also discussed returning half baked Entries and agreed to return nil instead.

However we use the invalid behavior of the previous get operation in later functions in Create(), so I retained old behavior for internal use in get() but made Get() comply with spec

Copy link
Contributor Author

@ripienaar ripienaar Oct 11, 2021

Choose a reason for hiding this comment

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

Since ErrKeyDeleted is now internal only maybe I should unexport it

@coveralls
Copy link

coveralls commented Oct 11, 2021

Coverage Status

Coverage increased (+0.3%) to 85.231% when pulling d7eb0d8 on ripienaar:get_nil_not_found into 41cb5bc on nats-io:main.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

js.go Outdated Show resolved Hide resolved
js.go Outdated Show resolved Hide resolved
@ripienaar
Copy link
Contributor Author

updated for _EMPTY_

Signed-off-by: R.I.Pienaar <rip@devco.net>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@ripienaar ripienaar merged commit 1655009 into nats-io:main Oct 11, 2021
@ripienaar ripienaar deleted the get_nil_not_found branch October 11, 2021 15:29
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.

None yet

3 participants