NetLogo / Tortoise

Compiler and runtime engine for NetLogo models that runs in JavaScript 🐢
https://netlogoweb.org
Other
56 stars 27 forks source link

Add Array Extension #217

Closed iloveivyxuan closed 4 years ago

iloveivyxuan commented 4 years ago

This PR is the array extension. All APIs are aligned with NetLogo Desktop, but also including one new API array:is-array which is inspired by nlmap extension.

iloveivyxuan commented 4 years ago

Thank you @LaCuneta for your detailed review and comments! I fixed the code. Following is what I've done.

Array extensions tests I moved array tests into a separate file, Array.txt.

Using the JS array as our backing data structure I was inspired by your example code and changed it a little bit. I defined an extArray constructor instead of defining a function and returning an object. I thought it would be readable and flexible to use a constructor here. (if I'm wrong, please correct me.)

Using workspace.dump() Thank you for noticing me the function workapce.dump(); I didn't realize this function means to print js types as NetLogo primitives. So I moved notArrayException() inside the init and use dump() to print array in a right way.

Adding unit tests to the desktop Array extension's tests.txt file Yeah, I can do this. Just a question. Is it to ensure that my array extension in NLW doesn't make any difference from NetLogo Desktop? So it would be better to also put unit tests inside desktop array tests.txt and make sure all tests get passed?

LaCuneta commented 4 years ago

This looks almost ready, thank you for your changes! I think the constructor is a good way to handle things.

The one thing I noticed was the from-list and to-list are returning the actual backing array, but this can lead to data leaks as the array extension object changes. I think we'd want to try a slice(0) on the backing array when we come from or to a NetLogo list in order to get a shallow copy that won't change (NetLogo lists are supposed to be immutable). Here are some tests to demonstrate that:

ArrayDoesNotLeak
  extensions [array]
  globals [arr ls]
  O> set ls ["oranges" "apples" "bananas"]
  O> set arr array:from-list ls
  O> array:set arr 0 "kiwi"
  ls => ["oranges" "apples" "bananas"]
  array:to-list arr => ["kiwi" "apples" "bananas"]
  O> set arr array:from-list [0 1 2 3]
  O> set ls array:to-list arr
  O> array:set arr 0 100
  ls => [0 1 2 3]
  array:to-list arr => [100 1 2 3]

Adding unit tests to the desktop Array extension's tests.txt file Yeah, I can do this. Just a question. Is it to ensure that my array extension in NLW doesn't make any difference from NetLogo Desktop? So it would be better to also put unit tests inside desktop array tests.txt and make sure all tests get passed?

That's right, if we have tests for desktop and web matching that gives us confidence they'll work the same. And the tests.txt you add to the Array extension will get run by NetLogo desktop during its development, as it is a bundled extension that we include as a git submodule. If you had another spot in mind to add the tests, just let me know.

A side note is that our automated CI tests through Jenkins are not updating the status of this PR with their results for some reason, but I've confirmed they are passing, so no worries there.

iloveivyxuan commented 4 years ago

Alright, I fixed it! Hope everything is okay now.

LaCuneta commented 4 years ago

Thank you for all your work on this!