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

join type and body don't match #65

Open
Ambrevar opened this issue Mar 24, 2021 · 2 comments
Open

join type and body don't match #65

Ambrevar opened this issue Mar 24, 2021 · 2 comments

Comments

@Ambrevar
Copy link

Ambrevar commented Mar 24, 2021

See

cl-str/str.lisp

Line 176 in 507ec6c

(defun join (separator strings)

I was recently bit this (see atlas-engineer/nyxt#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 51feb40#diff-8793bcbd16d1dc95956881ad175e7b1ad86aabfbdbf037975049fd71200af390 the function type only allows list of strings, while the body has remained the same.

So I suggest

  • either changing the type to accept alist arg
  • or changing the body to (apply #'concatenate 'string ARG) to match the new signature.
    Thoughts?

(Edit: Typos.)

@vindarel
Copy link
Owner

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: #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
Copy link
Author

Ambrevar commented Mar 24, 2021

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:

  • tweak the argument type (but keep the return type);

  • change the body because format is useless here; currently the body
    would leave the reader to believe that the function is meant to
    accept list of T, but the type contradicts this.

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)

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

No branches or pull requests

2 participants