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

trying to add indexOf to array #1352

Merged
merged 21 commits into from
Feb 23, 2022
Merged

trying to add indexOf to array #1352

merged 21 commits into from
Feb 23, 2022

Conversation

bjartek
Copy link
Contributor

@bjartek bjartek commented Jan 11, 2022

Closes #1276

Description

If you want to remove an element from a cadence array right now you have to use loops in cadence. This is inneficient.
This PR implements the indexOf method that in combination with remove can do that.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice work, looks good! Maybe rename, and please also add interpreter tests

@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2022

Codecov Report

Merging #1352 (b6b54cd) into master (6e61ec4) will decrease coverage by 3.16%.
The diff coverage is 98.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1352      +/-   ##
==========================================
- Coverage   75.81%   72.65%   -3.17%     
==========================================
  Files         279      288       +9     
  Lines       38780    39301     +521     
==========================================
- Hits        29402    28555     -847     
- Misses       8016     9257    +1241     
- Partials     1362     1489     +127     
Flag Coverage Δ
unittests 72.65% <98.33%> (-3.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/interpreter/value.go 55.90% <96.29%> (-15.39%) ⬇️
runtime/sema/type.go 88.43% <100.00%> (-0.03%) ⬇️
runtime/compiler/ir/func.go 0.00% <0.00%> (-100.00%) ⬇️
runtime/compiler/wasm/modulebuilder.go 0.00% <0.00%> (-96.30%) ⬇️
runtime/compiler/codegen.go 0.00% <0.00%> (-79.55%) ⬇️
runtime/compiler/ir/expr.go 0.00% <0.00%> (-50.00%) ⬇️
runtime/compiler/ir/constant.go 0.00% <0.00%> (-50.00%) ⬇️
runtime/compiler/ir/stmt.go 0.00% <0.00%> (-33.34%) ⬇️
runtime/errors.go 37.71% <0.00%> (-13.59%) ⬇️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e61ec4...b6b54cd. Read the comment docs.

@turbolent turbolent self-assigned this Jan 12, 2022
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

@bjartek Thank you for updating and adding more tests. There are some (minor) whitespace issues and it would be great if you could add one more checker test case for non-equatable values, then it should be good to go!

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Looks great, nice work! 👏

@SupunS Could you please also have a look?

Comment on lines +770 to +783
func TestCheckArrayIndexOfNonEquatableValueArray(t *testing.T) {

t.Parallel()

_, err := ParseAndCheck(t, `
fun test(): Int? {
let x = [[1, 2], [3]]
return x.firstIndex(of: [3])
}
`)

errs := ExpectCheckerErrors(t, err, 1)
assert.IsType(t, &sema.NotEquatableTypeError{}, errs[0])
}
Copy link
Member

Choose a reason for hiding this comment

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

👌

@turbolent
Copy link
Member

@SupunS @dsainati1 when you get a change, could you please have a second look? Thank you!

Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Looks good! 🎉

Thank you for the patient on the long delay for the review 🙏

Co-authored-by: Supun Setunga <[email protected]>
@turbolent turbolent merged commit 7a379bb into onflow:master Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array indexOf implementation
4 participants