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.

Sign up for GitHub

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

fix KVOp types #2531

Merged
merged 1 commit into from
Dec 1, 2016
Merged

fix KVOp types #2531

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions api/kv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,21 +50,21 @@ type KVOp string

const (
KVSet KVOp = "set"
KVDelete = "delete"
KVDeleteCAS = "delete-cas"
KVDeleteTree = "delete-tree"
KVCAS = "cas"
KVLock = "lock"
KVUnlock = "unlock"
KVGet = "get"
KVGetTree = "get-tree"
KVCheckSession = "check-session"
KVCheckIndex = "check-index"
KVDelete KVOp = "delete"
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to repeat the KVOp here, the first one will set them all. It was the change to the struct below that fixed this.

Copy link
Author

@alicebob alicebob Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

afaik that only works for iota assignments. This is the output of gist https://gist.github.com/alicebob/aa17022b6f1d6f8fbf3337e2c178c858

KVSet: main.KVOp
KVDelete: main.KVOp
KVDeleteCAS: string
KVDeleteTree: string

Copy link
Author

@alicebob alicebob Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The relevant piece from the Go language specs:

Within a parenthesized const declaration list the expression list may be omitted from any but the first declaration. Such an empty list is equivalent to the textual substitution of the first preceding non-empty expression list and its type if any. Omitting the list of expressions is therefore equivalent to repeating the previous list.

https://golang.org/ref/spec#Constant_declarations

Since the expression isn't empty (it's a string) you need to repeat the type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @alicebob that clause in the spec is to allow for iota, basically. The section before that talks about the type being optional, not the expression part. I tried it in the Go playground and it worked fine with just the one type, and we do that in lots of other places in Consul :-)

Copy link
Author

Choose a reason for hiding this comment

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

Morning @slackpad. Sorry to make such a fuzz about a few lines, but it's not the same thing. The types don't get copied over; it happens to work because the definitions without a type are untyped string constants. Not specifying a type at all also works fine:

type KVOp string

const (
    KVSet          = "set"
    KVDelete       = "delete"
...

If you only add a KVOp type for KVSet then KVSet will be a typed string, but KVDelete will still be untyped. So I would either add the type on every line, or not at all.

Have a look at my Gist posted above, and reread the 'String constants' part from https://blog.golang.org/constants .

Copy link
Contributor

Choose a reason for hiding this comment

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

Doh thanks for the correction and sorry I missed the gist output above - my understanding was definitely incorrect!

KVDeleteCAS KVOp = "delete-cas"
KVDeleteTree KVOp = "delete-tree"
KVCAS KVOp = "cas"
KVLock KVOp = "lock"
KVUnlock KVOp = "unlock"
KVGet KVOp = "get"
KVGetTree KVOp = "get-tree"
KVCheckSession KVOp = "check-session"
KVCheckIndex KVOp = "check-index"
)

// KVTxnOp defines a single operation inside a transaction.
type KVTxnOp struct {
Verb string
Verb KVOp
Key string
Value []byte
Flags uint64
Expand Down