Skip to content

tests: Add tests for StaticArray #1728

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

Merged
merged 1 commit into from
Mar 22, 2021
Merged

tests: Add tests for StaticArray #1728

merged 1 commit into from
Mar 22, 2021

Conversation

saulecabrera
Copy link
Contributor

Adds the following missing tests for StaticArray:

  • constructor

  • fromArray

  • concat (static)

  • slice (static)

  • concat (instance)

  • includes

  • indexOf

  • join + toString

  • I've read the contributing guidelines

@saulecabrera
Copy link
Contributor Author

saulecabrera commented Mar 11, 2021

@dcodeIO @MaxGraey one thing that I noticed while working on this is that we have isArray for which the documentation states the following:

Tests if the specified type or expression can be used as an array. Compiles to a constant.

But isArray returns false with StaticArray, which can be used as an array (I know that it technically doesn't extend it) but this seems inconsistent/counterintuitive. Should this be fixed in the docs?

There's also isArrayLike, and in this case, isArrayLike returns true for a StaticArray, but there is no documentation for this one (maybe I missed it 🤷‍♂️), is there a reason for this? Or should we document it?

Adds the following missing tests for StaticArray:

- constructor
- fromArray
- concat (static)
- slice (sstatic)
- concat (instance)
- includes
- indexOF
- join + toString
@saulecabrera
Copy link
Contributor Author

@dcodeIO 👋 any thoughts on this?

@dcodeIO
Copy link
Member

dcodeIO commented Mar 17, 2021

It's a bit unfortunate that StaticArray is somewhat incompatible with normal Arrays, even though that's about the point to introduce it. My immediate thought was that we should have named it differently, something without an Array in it perhaps, but now we are kinda stuck with it. So, to be honest, I don't know what to do, but documenting the difference certainly makes sense :)

@saulecabrera
Copy link
Contributor Author

So, to be honest, I don't know what to do, but documenting the difference certainly makes sense :)

That's a valid outcome of this discussion, I think that as long as the differences are pointed out correctly then the name becomes less relevant. That said, I didn't intend to make my comment above a blocker to merge this, it was mostly observation. I'll follow up with a documentation issue/PR.

@dcodeIO dcodeIO requested a review from MaxGraey March 22, 2021 22:05
@dcodeIO dcodeIO merged commit 231a8dc into AssemblyScript:master Mar 22, 2021
@dcodeIO
Copy link
Member

dcodeIO commented Mar 22, 2021

Thanks! :)

@saulecabrera saulecabrera deleted the add-tests-for-static-array branch April 19, 2021 11:30
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.

3 participants