add collect with infered init, refs #16078 fixes #14332

[planetis-m]

Supersedes #16082, refs #16078. it is not injected. Reason is, if you write it[i] = a then there is no point using collect, is there? :P

[planetis-m]

It imports tables and sets, maybe inferring container type should work for seqs only?

[planetis-m]

Idea: use newSeq and produce a seq[(K, V)] Or make it an error.

[planetis-m]

On the other hand, it's not worth it: you can do collect(for i, x in a: (i, x))

[planetis-m]

Is that better?

      when (NimMajor, NimMinor) >= (1, 5):
        bracketExpr.add(bindSym"initTable")
      else: error("Table type is not specified")

[planetis-m]

Same, use newSeq and ignore {a}

[planetis-m]

For now I just used ident"initTable" and avoided the import.

[planetis-m]

@moigagoo can you take a look please? For a simple example, I can't reproduce.

import strutils
    let x = collect:
      for d in data.items:
        "bird $1" % [d]
    assert x == @["bird bird", "bird word"]

[moigagoo]

@planetis-m Hi! Sorry, I don't quite follow. Take a look at what exactly? What is it you can't reproduce?

[planetis-m]

@moigagoo this build failure:

  /home/runner/work/Nim/Nim/pkgstemp/norm/tests/sqlite/trows.nim(16, 7) template/generic instantiation of `suite` from here
  /home/runner/work/Nim/Nim/pkgstemp/norm/tests/sqlite/trows.nim(65, 8) template/generic instantiation of `test` from here
  /home/runner/work/Nim/Nim/pkgstemp/norm/tests/sqlite/trows.nim(70, 10) template/generic instantiation of `with` from here
  /home/runner/work/Nim/Nim/pkgstemp/norm/tests/sqlite/trows.nim(72, 13) template/generic instantiation of `select` from here
  /home/runner/work/Nim/Nim/pkgstemp/norm/src/norm/sqlite.nim(155, 24) template/generic instantiation of `collect` from here
  /home/runner/work/Nim/Nim/pkgstemp/norm/src/norm/sqlite.nim(157, 41) Error: type mismatch: got <>
  but expected one of: 
  proc (s: var seq[string], len: Natural){.noSideEffect.}

[moigagoo]

@planetis-m

So, AFAIU you've modified the behavior of collect macro and this change broke Norm at least in one place where collect is used. Is this correct?

I suggest you try running simple tests like this one with your modified version and see if it runs: https://play.nim-lang.org/#ix=2EYf

[planetis-m]

no it wasn't modified I added a second macro, it's hidden under a since(1, 5)

    let x = collect(newSeq):
      for d in data.items:
        "bird $1" % [d]
    assert x == @["bird bird", "bird word"]

runs fine.

@moigagoo if you can nimble install looper and import sugar except collect; import looper, then run tests with --expandmacro:collect that would be appreciated.

Or just run with --expandmacro:collect

[moigagoo]

@planetis-m Well I have no idea why it could break ¯_(ツ)_/¯ Try wrapping it in a unittest test macro to replicate the code that is actually being run and that actually fails. Maybe the new version somehow doesn't play well within macros.

A few points:

1. I haven't read your PR and I don't really plan to. So, I can't see exactly what could go wrong.
2. Since it's your change to the stdlib, I stand with the belief that it is your responsibility to make it work with existing packages.
3. Installing a customized version of Nim and running and debugging Norm tests with it is tedious and it really isn't how I plan to spend this beautiful Saturday evening :-)

Sorry for not being helpful, and I really hope I don't sound like an asshole. Just protecting my personal time.

[planetis-m]

You make good points. However your package is broken, my PR isn't related. Once you get the time, fix it please. Since it's my responsibility for fixing the stlib, its yours to fix your package :)

[moigagoo]

your package is broken, my PR isn't related.

I disagree. The tests pass with a PR that has been pushed after yours: https://github.com/nim-lang/Nim/runs/1435997008?check_suite_focus=true#step:11:519

[planetis-m]

Checked the produced ast, PR changed some nimnodes from ident to sym: https://play.nim-lang.org/#ix=2EYD

[planetis-m]

Can I move collect in a separate module like std/collect? Edit: only if we deprecate sugar.collect

[planetis-m]

This can use a review, thank you!