Nim icon indicating copy to clipboard operation
Nim copied to clipboard

added `sequtils.mapItLit`, replaces `mapLiterals`

Open timotheecour opened this issue 3 years ago • 12 comments

  • mapItLit is error prone and not flexible enough, see https://github.com/nim-lang/Nim/pull/18577#issuecomment-886231715
  • mapIt returns a seq so isn't appropriate in cases where you want to return an array|set|etc

This PR introduces mapItLit which is more useful and intuitive than mapLiterals, see examples; lots of examples wouldn't work with mapLiterals, which is just the wrong abstraction

example 0:

mapLiterals can't handle simple transformations

import sequtils
# echo mapLiterals([(1, 'a'), (2, 'b')], ??) # impossible
echo mapItLit([(1, 'a'), (2, 'b')], (it[0].float, it[1])) # [(1.0, 'a'), (2.0, 'b')]

example 1:

mapLiterals doesn't work with user defined literals

import sequtils, strutils
proc `'big`(a: string): int = a.parseInt
echo mapLiterals((2, 3'big), `$`) # ("2", 3) instead of ("2", "3")
echo mapItLit((2, 3'big), $it) # ("2", "3")

example 2:

mapLiterals doesn't work with const elements

import sequtils, strutils, math
const a = 3
echo mapLiterals((PI, Inf, a, 4), `$`) # (3.141592653589793, inf, 3, "4")
echo mapItLit((PI, Inf, a, 4), $it) # ("3.141592653589793", "inf", "3", "4")

example 3:

mapLiterals can't reuse a const array-like literal

import sequtils
const vals = [1,2,3]
echo mapLiterals(vals, float) # [1, 2, 3]
echo mapLiterals(vals, int8) # [1, 2, 3] (not actually transformed)

echo mapItLit(vals, it.float) # [1.0, 2.0, 3.0]
echo mapItLit(vals, it.int8) # [1, 2, 3]

example 4:

mapLiterals requires an auxiliary routine whereas mapItLit can inline the expression:

import sequtils
template doubleIt(it): untyped = (it, it)
echo mapLiterals([1,2,3], doubleIt)

echo mapItLit([1,2,3], (it, it))
echo mapItLit([1,2,3], doubleIt(it)) # also works

timotheecour avatar Jul 27 '21 22:07 timotheecour

How about putting it in a library first?

disruptek avatar Jul 28 '21 01:07 disruptek

it's intended as a replacement for mapLiterals which is already in sequtils; the 4 examples showing limitations of mapLiterals are IMO a strong enough reason; require a 3rd party library will deter usage.

timotheecour avatar Jul 28 '21 21:07 timotheecour

Some questions:

  • Is there a reason mapLiterals can't be expanded to incorporate this functionality?
  • Wouldn't mapExpr and mapItExpr be better? A literal is generally an atomic language unit that represents some static value. Since this procedure works on constant expressions as well as literals, having the name single out literals is inaccurate.

Varriount avatar Jul 28 '21 22:07 Varriount

Is there a reason mapLiterals can't be expanded to incorporate this functionality?

that would be a breaking change, and would not reflect the semantics of mapItLit, which doesn't care about literals vs non-lilterals elements (nor does it care about const-ness of elements btw)

Since this procedure works on constant expressions as well as literals,

mapItLit doesn't care about const either, it works with RT values, eg:

import sequtils
var x = 10
echo mapItLit([x, x + 1, 5], it*2) # [20, 22, 10]

having the name single out literals is inaccurate.

the lit in the name stands for the fact that, unlike mapIt, the macro returns a literal AST (which itself may refer to RT values, as shown above); mapItExpr doesn't quite convey this

timotheecour avatar Jul 28 '21 23:07 timotheecour

the lit in the name stands for the fact that, unlike mapIt, the macro returns a literal AST (which itself may refer to RT values, as shown above); mapItExpr doesn't quite convey this

Yes, but lit doesn't convey this either. The terms "literal" and "lit" do not mean "an untyped AST", they mean "a fixed value (such as a string, number, or structure) in source code".

Varriount avatar Jul 28 '21 23:07 Varriount

how about mapItContainer ?

timotheecour avatar Jul 28 '21 23:07 timotheecour

how about mapItContainer ?

Since it maps directly on the AST, I think mapItAst would be a good name.

konsumlamm avatar Jul 29 '21 07:07 konsumlamm

I don't know what is wrong with mapLiterals that it needs fixing. I mean, yeah it sucks, but it sucked for 4 years already and nobody cared. Leaving old code alone works just fine. Nobody is forced to use it and it works just fine for the cases that were actually documented. "I'm confused and I don't like it and it doesn't work for my undocumented use cases" is not a bug.

Araq avatar Jul 29 '21 09:07 Araq

I've now removed the {.deprecated.} mention.

but it sucked for 4 years already and nobody cared.

there are 2 types of APIs: the ones people complain about and the ones nobody uses. mapLiterals was very seldom used according to github (across all repos) because its semantics make it rarely useful in practice, as shown in top examples.

timotheecour avatar Jul 29 '21 10:07 timotheecour

I've now removed the {.deprecated.} mention.

but it sucked for 4 years already and nobody cared.

there are 2 types of APIs: the ones people complain about and the ones nobody uses. mapLiterals was very seldom used according to github (across all repos) because its semantics make it rarely useful in practice, as shown in top examples.

I use it when writing web code mostly in arrays of string literals to transform them to cstring, like this:

mapLiterals(["abc", "def"], cstring) I have no serious complaints.

planetis-m avatar Jul 30 '21 10:07 planetis-m

After some thought, I don't think this is worth adding. The only problems I've run into with mapLiteral are when I was using it for input it wasn't meant to apply to in the first place (which I do consider a bug).


The real change I would prefer to see here is having map/mapIt renamed to mapToSeq/mapThisToSeq*, and having map be an iterator. Then, the name mapToLiteral would actually make sense, as you would be mapping an input literal** to an output literal**.

*I don't like It, I feel that it's too easy to miss when reading code. **A sequence/tuple/array literal.

Varriount avatar Jul 31 '21 23:07 Varriount

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

stale[bot] avatar Aug 10 '22 04:08 stale[bot]