vindarel / cl-str

Modern, simple and consistent Common Lisp string manipulation library.
https://vindarel.github.io/cl-str/
MIT License
305 stars 37 forks source link

`join` type and body don't match #65

Open Ambrevar opened 3 years ago

Ambrevar commented 3 years ago

See https://github.com/vindarel/cl-str/blob/507ec6c6ec150f467fbf776f578af5b2d9b67caf/str.lisp#L176

I was recently bit this (see https://github.com/atlas-engineer/nyxt/issues/1217 if you want to get confused :p): join used to accept a list of anything and leverage format to turn the list elements to strings using print-object.

But since https://github.com/vindarel/cl-str/commit/51feb409c93bc1035e3edf4f47a61c402d02b7ec#diff-8793bcbd16d1dc95956881ad175e7b1ad86aabfbdbf037975049fd71200af390 the function type only allows list of strings, while the body has remained the same.

So I suggest

(Edit: Typos.)

vindarel commented 3 years ago

So the issue is that (str:join " " '(17)) blows up:

The value
  (17)
is not of type
  (OR NULL (CONS STRING T))
when binding STR::STRINGS
   [Condition of type TYPE-ERROR]

Which makes sense.

You fixed it with

(str:join " " (mapcar #'princ-to-string args))

You also wanted type declarations for join: https://github.com/vindarel/cl-str/issues/51

either changing the type to accept alist arg

a list of anything? Would that be compatible with your previous request?

changing the body to (apply #'concatenate 'string ARG) to match the new signature.

you mean to simplify the body?

When tried at the REPL, it blows before entering the function. Is there a way to keep the type declarations, but still allow to run the function with any arguments?

Also we could accept a list of numbers:

(declaim (ftype (function ((or null character string)
                           (or null
                               (cons string)
                               (cons number)
                               ))
                          string)
                join))

but… nah, that's smelly.

Ambrevar commented 3 years ago

To clarify: I wanted the type declaration for the return value.

I initially didn't know that str:join (from version 0.19) accepted lists of anything.

Now that I see it does, I can think of two options:

either changing the type to accept alist arg

a list of anything? Would that be compatible with your previous request?

Yes, since my previous request was about the return type.

changing the body to (apply #'concatenate 'string ARG) to match the new signature.

you mean to simplify the body?

When tried at the REPL, it blows before entering the function. Is there a way to keep the type declarations, but still allow to run the function with any arguments?

That's because the type declaration is not approriate for this.

Also we could accept a list of numbers:

(declaim (ftype (function ((or null character string)
                           (or null
                               (cons string)
                               (cons number)
                               ))
                          string)
                join))

but… nah, that's smelly.

Indeed, one may ask "why numbers and not symbols or characters?" I think it's better to stick to an "everything or nothing" logic.

Here are my suggestions.

  1. Accept list of T:
(declaim (ftype (function (string list) string) join))
; Same function.
  1. Or accept list of strings only:
(declaim (ftype (function ((or null character string)
                           (or null (cons string)))
                          string)
                join))
(defun join (separator strings)
  "Join all the strings of the list with a separator."
  (apply #'concatenate 'string (serapeum:intersperse separator strings)))

Thoughts?

(Edit: Typos)