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

Added toarray alias for totable #77

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

svermeulen
Copy link

I usually hear the word "table" as a reference to any lua tables generically rather than one that is meant to be used as a sequential set of data, so thought this would make sense as an alias

@igormunkin
Copy link

@svermeulen, thanks for your patch! I would like to share some thoughts regarding the naming.

TL;DR: tosequence/toseq aliases looks better (at least to me) if you need another one for totable (I doubt you do).


Strictly saying, there are no arrays in Lua, but I see your point regarding the naming: you would like to emphasis the fact there is no "holes" in numerical part of Lua table, hence the length value of the table is defined and correct. Unfortunately, such a structure is actually called "array" and even "array with/without holes" in Lua 5.1 Reference manual. However, starting from Lua 5.2 it is called a "sequence", that (IMHO) is way much better naming for this particular sort of Lua tables.

My reasoning follows this rule of thumb: array (as a continuous block of homogeneous items) is always considered with its length or capacity; sequence is a continuous number of elements with no terminator (i.e. nil) in the middle of it, that sounds closer to single-linked list (just consider the way table.maxn is calculated in LuaJIT).

All in all, everything above are just my perception of the Lua world, so if somebody with the power of merge is OK with your naming, I can't insist on mine. "You must choose, but choose wisely" ;)


And of course, please add some tests for your alias and update the docs, otherwise it will be broken or unused in the future.

Copy link

@igormunkin igormunkin left a comment

Choose a reason for hiding this comment

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

@svermeulen, thanks for the fixes and sorry for stalling the discussion.

I still have a couple of nits:

  • It's better to squash all the commits into the single one with a nice commit message.
  • The test for toseq alias is still missing. I'd rather implement the test checking the fact that both toseq and tosequence are aliases for totable instead of writing functional tests for them.
  • Docs still say not a word regarding the new aliases.

Otherwise, I have no objections and ask to proceed the second reviewer with the patch.

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

2 participants