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

Split out ArrayIter from vim.iter() #28941

Open
lewis6991 opened this issue May 23, 2024 · 5 comments
Open

Split out ArrayIter from vim.iter() #28941

lewis6991 opened this issue May 23, 2024 · 5 comments
Labels
enhancement feature request lua stdlib
Milestone

Comments

@lewis6991
Copy link
Member

lewis6991 commented May 23, 2024

Problem

Do we still think it was the right choice to make vim.iter handle arrays and general tables together? Could we consider splitting this (as Lua does with pairs and ipairs), so the right behaviour can be opted in appropriately so it is less confusing?

E.g. I was experimenting with vim.iter today and wanted to port:

    local unmatched_msg = table.concat(
      vim.tbl_map(function(v)
        return string.format('    - %s', v.text or v)
      end, spec),
      '\n'
    )

which with vim.iter is:

    local unmatched_msg = vim.iter(lines):map(function(v)
      return string.format('    - %s', v.text or v)
    end):join('\n')

However, it really wasn't obvious to me that the map function only takes a single argument, unlike ipairs which gives you both the index and the value.

I know vim.iter(pairs(t)) is a thing, but it's just an unnecessary footgun and vim.iter() behaving very differently depending on the shape of the keys seems problematic to me. The user may have a table that just happens to be indexed my integers but should be treated as a regular table, e.g. for a table<id: integer, object: table> type.

Expected behavior

I propose that vim.iter(t) == vim.iter(pairs(t)) and we add something like vim.arrayiter(t) so the specialized behaviour can be opted in. This would also allow us to:

  • remove the extra table allocation and scan. The user tells us how to iterate, we do not try to infer.
  • remove the polymorphic return type of vim.iter() thereby making all methods of vim.iter(...) have the same signature.

Overall I think removing some of this extra smartness will simplify the mental model of vim.iter which for me is currently a bit overloaded.

@justinmk
Copy link
Member

justinmk commented May 23, 2024

Note: review #24746 before commenting here

Having a way to tell vim.iter "treat this like a dict/list/array" seems useful. This be a thin wrapper like vim.iter.dict/vim.iter.list (or vim.dict/vim.list) which just skips the "sniffing" logic of vim.iter.

it really wasn't obvious to me that the map function only takes a single argument, unlike ipairs which gives you both the index and the value.

Yeah, I also advocated for having the same signature for the callbacks, regardless of list vs dict. There were valid arguments for the current behavior, but I still prefer the predictable and consistent signature.

@lewis6991

This comment was marked as outdated.

@lewis6991 lewis6991 changed the title Split out ArrayIter from vim.iter Split out ArrayIter from vim.iter() May 23, 2024
@justinmk
Copy link
Member

with this a user could add local pairs = vim.iter.pairs and it work as normal?

Not in favor of that. It would be annoying if the builtin pairs is needed in the same module. And vim.iter(pairs(..)) is perfectly reasonable.

I really don't want to get hung up on having one all knowing interface, there's a reason Lua split these out into ipairs and pairs.

To make progress, let's be very precise and concrete. Currently, we've identified that an opt-in is needed. Let's focus on that rather than more general statements about an "all knowing interface".

@lewis6991
Copy link
Member Author

lewis6991 commented May 23, 2024

Not in favor of that. It would be annoying if the builtin pairs is needed in the same module. And vim.iter(pairs(..)) is perfectly reasonable.

If so, then yes it's not a good idea. However, I don't think it would ever be needed. From what I can tell it's basically a drop in replacement with additional functionality (:fold, :map).

I think we also need specialized versions so :totable() works properly. vim.iter(pairs()) is indistinguishable from the general vim.iter(<function>). Specifically, I think we want:

  • vim.iter.pairs({1,2,3,nil,5,key=value}):totable() == {1,2,3,nil,5,key=value})
    • and generally vim.iter.pairs(t):totable() == t
  • vim.iter.ipairs({1,2,3,nil,5,key=value}):totable() == {1,2,3})
  • vim.iter.array({1,2,3,nil,5,key=value}):totable()== {1,2,3,nil,5})
    • I think this is right? Or is it {1,2,3,5}?
  • vim.iter(<function>) generally returns { {v_1_1, v_1_2, ...}, {v_2_1, v_2_2, ...}, ...} (as it does now)
    • vim.iter(pairs({1,2,3,nil,5,key=value})):totable() == {{1, 1}, {2, 2}, {3, 3}, {5, 5}, {key, value}}
    • vim.iter(ipairs({1,2,3,nil,5,key=value})):totable() == {{1, 1}, {2, 2}, {3, 3}}
  • vim.iter(<table>):
    • I don't know of a good choice here so my preference would be to not allow it.

And overall we do not make any assumptions about how a table is expected to be iterated. Just like with ipairs and pairs it must be explicit.

To make progress, let's be very precise and concrete. Currently, we've identified that an opt-in is needed. Let's focus on that rather than more general statements about an "all knowing interface".

Ok, let's start with the removal/deprecation of the sniffing logic.

@justinmk
Copy link
Member

let's start with the removal/deprecation of the sniffing logic.

Starting with an opt-in is much lower risk and more tractable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature request lua stdlib
Projects
None yet
Development

No branches or pull requests

2 participants