Navigation Menu

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

builtin delete for maps and arrays #250

Merged
merged 10 commits into from Feb 19, 2020
Merged

builtin delete for maps and arrays #250

merged 10 commits into from Feb 19, 2020

Conversation

ozanh
Copy link
Contributor

@ozanh ozanh commented Feb 16, 2020

builtin delete is ready. It works like Go's delete for maps but also works for array types. delete function returns only undefined value or raises runtime error. It mutates given map or array. doc and tests are added.

Ozan HACIBEKİROĞLU and others added 7 commits February 12, 2020 15:20
@ozanh ozanh mentioned this pull request Feb 16, 2020
@d5
Copy link
Owner

d5 commented Feb 16, 2020

Right now Tengo does not support negative indexing into arrays. See https://github.com/d5/tengo/blob/master/objects.go#L223 and https://github.com/d5/tengo/blob/master/objects.go#L873. So I think we should remove it from this PR for consistent behavior.

@ozanh
Copy link
Contributor Author

ozanh commented Feb 16, 2020

I will remove it soon you are right I missed it

@geseq
Copy link
Collaborator

geseq commented Feb 18, 2020

A builtin for map deletes makes perfect sense, but now that I think about it, for arrays, the go syntax of creating slices in the form a[i:j] might be better than a delete function. I'm gonna look into this further tomorrow.

@ozanh
Copy link
Contributor Author

ozanh commented Feb 18, 2020

Daniel, I thought that for a scripting language such a syntax may not be appealing; a = a[:2] + a[3:] if item is in the middle and garbage stays in the slice until the script ends (not a big problem though). Honestly, my future rule engine users, who won't be Gophers, can be confused, I guess.

@ozanh
Copy link
Contributor Author

ozanh commented Feb 18, 2020

If tengo will have methods like Array.pop(index) or Array.splice(...) etc. in the future, delete builtin should not support arrays. There must be only one way to do it.

@d5
Copy link
Owner

d5 commented Feb 18, 2020

Actually I really like the idea of having a separate builtin function of splice for array types. That way we can maintain the expected behavior of delete (that works for maps) and users can still choose to use splice for array manipulations. Thoughts? @geseq @ozanh

@ozanh
Copy link
Contributor Author

ozanh commented Feb 18, 2020

It sounds good to me.

@geseq
Copy link
Collaborator

geseq commented Feb 19, 2020

Yeah splice sounds good.

@ozanh
Copy link
Contributor Author

ozanh commented Feb 19, 2020

I will remove array support from delete function if you are OK. Later I am going to implement a splice builtin. Guide me please.

@geseq
Copy link
Collaborator

geseq commented Feb 19, 2020

Perfect!

@geseq geseq merged commit ac53405 into d5:master Feb 19, 2020
@ozanh ozanh mentioned this pull request Feb 25, 2020
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