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

fix KVOp types #2531

Merged
merged 1 commit into from
Dec 1, 2016
Merged

fix KVOp types #2531

merged 1 commit into from
Dec 1, 2016

Conversation

alicebob
Copy link

It was not possible to use KVSet as a transaction type:

./main.go:94: cannot use api.KVSet (type api.KVOp) as type string in field value

Copy link
Contributor

@slackpad slackpad 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 PR! Noted one small thing.

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

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

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!

@slackpad slackpad added this to the 0.7.2 milestone Nov 29, 2016
@slackpad slackpad merged commit 0282dd9 into hashicorp:master Dec 1, 2016
@alicebob alicebob deleted the txntypes branch December 1, 2016 15:52
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