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

added a has-key and has-value builtin to test the existence of an element. #432

Merged
merged 1 commit into from Jul 13, 2017

Conversation

Projects
None yet
2 participants
@HeavyHorst
Contributor

HeavyHorst commented Jul 12, 2017

This works with maps and every iterable type.

~/go/src/github.com/elves/elvish> contains [1 2 "3"] 2
▶ $true
~/go/src/github.com/elves/elvish> contains [1 2 "3"] 5
▶ $false
~/go/src/github.com/elves/elvish> contains [&foo=bar &lorem=ipsum] "lorem"
▶ $true
~/go/src/github.com/elves/elvish> contains "hallo" "o"
▶ $true

@xiaq what do you think?

fixes #407 #398

@xiaq

This comment has been minimized.

Member

xiaq commented Jul 12, 2017

This has the same problem with Python's in operator: it tests for presence of values for lists, and the presence of keys for maps. This former is more obvious, but the latter is not.

It's better to have a pair of builtins has-key and has-value to make this explicit. It would also be desirable if has-key handles slice index for lists. Examples:

~> li = [lorem ipsum foo bar]
~> map = [&lorem=ipsum &foo=bar]
~> has-key $li 0
$true
~> has-key $li 1:3 # slice for lists
$true
~> has-key $li 100
$false
~> has-key $m haha
$false
~> has-key $m lorem
$true
~> # maps don't have slice indices

You can use ParseAndFixListIndex to parse list indices, using util.PCall to catch the "exception".

The has-value builtin should hopefully be obvious.

@HeavyHorst HeavyHorst changed the title from added a contains builtin to test the existence of an element. to added a has-key and has-value builtin to test the existence of an element. Jul 12, 2017

@HeavyHorst HeavyHorst force-pushed the HeavyHorst:contains branch from 30e9ad9 to 860897e Jul 12, 2017

@HeavyHorst

This comment has been minimized.

Contributor

HeavyHorst commented Jul 12, 2017

I've splitted the logic into has-key and has-value as requested.

~/go/src/github.com/elves/elvish> li = [lorem ipsum foo bar]
~/go/src/github.com/elves/elvish> map = [&lorem=ipsum &foo=bar]
~/go/src/github.com/elves/elvish> has-key $li 0
▶ $true
~/go/src/github.com/elves/elvish> has-key $li 1:3
▶ $true
~/go/src/github.com/elves/elvish> has-key $li 100
▶ $false
~/go/src/github.com/elves/elvish> has-key $map haha
▶ $false
~/go/src/github.com/elves/elvish> has-key $map lorem
▶ $true

@xiaq

Thanks for the update! Please see the comments.

Also, please also add tests to the evalTests list in eval_test.go, like the following:

{`has-key [foo bar] 0`, bools(true), nomore},
ec.ports[1].Chan <- Bool(found)
}
func hasKey(ec *EvalCtx, args []Value, opts map[string]Value) {

This comment has been minimized.

@xiaq

xiaq Jul 12, 2017

Member

Use ScanArgs.

v := args[0]
switch val := v.(type) {
case HasKeyer:
if val.HasKey(args[1]) {

This comment has been minimized.

@xiaq

xiaq Jul 12, 2017

Member

if x { found = true } is equivalent to found = x.

found = true
}
case Lener:
if err := util.PCall(func() {

This comment has been minimized.

@xiaq

xiaq Jul 12, 2017

Member

Same; just use found = ...

@@ -780,6 +782,57 @@ func rangeFn(ec *EvalCtx, args []Value, opts map[string]Value) {
}
}
func hasValue(ec *EvalCtx, args []Value, opts map[string]Value) {
TakeNoOpt(opts)

This comment has been minimized.

@xiaq

xiaq Jul 12, 2017

Member

You can declare var container, key Value and use ScanArgs here. ScanArgs also check the number of arguments.

if val.HasKey(args[1]) {
found = true
}
case Lener:

This comment has been minimized.

@xiaq

xiaq Jul 12, 2017

Member

Hmm, this is actually a bit brittle as Lener does not always accept numerical indices (e.g. maps also implement Lener). But I cannot think of a better solution at the moment, please add a XXX comment here like XXX(xiaq): Not all types that implement Lener have numerical indices

}
return true
})
} else {

This comment has been minimized.

@xiaq

xiaq Jul 12, 2017

Member

It would be great if has-value also works for MapLike types.

var found bool
if len(args) == 2 {
v := args[0]
switch val := v.(type) {

This comment has been minimized.

@xiaq

xiaq Jul 12, 2017

Member

You can reuse the name v: v := v.(type)

@HeavyHorst HeavyHorst force-pushed the HeavyHorst:contains branch from 860897e to e8f1ed9 Jul 13, 2017

@HeavyHorst

This comment has been minimized.

Contributor

HeavyHorst commented Jul 13, 2017

Done. :)

@xiaq xiaq merged commit ac86bde into elves:master Jul 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@HeavyHorst HeavyHorst deleted the HeavyHorst:contains branch Jul 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment