duplicate concat as a replacement for append

[pfiaux]

Draft for #114, started with a naive approach, duplicate (append) and rename it (concat). Then Replaced uses of append with concat and duplicated go test of append for concat.

• Implement (concat) based on
• Replace uses of (append) with (concat)
• Update tests
• Add in code deprecation warning
• Double check uses of append in existing scripts

So far so good. In terms of deprecation imaging the following points need to be handled:

• Is there a way (append) can print a warning? does that even make sense? (ideally it would do so once per program execution not per function execution).
• Should (append) be updated to use (concat)'s implementation, or is it better to keep it as a duplicate because it's short.

Bare with me I'm still brushing up on my LISP, but feel free to let me know if something is obviously wrong.

[vito]

Thanks for working on this! :)

Is there a way (append) can print a warning? does that even make sense? (ideally it would do so once per program execution not per function execution).

That's an interesting idea. Not sure how to implement it yet, but here's a thought: we could start using metadata to annotate deprecated things with a message instructing what to do instead.

; concatenates things
(def concat 42)

^{:deprecated "Use (concat) instead."}
(def append concat)

(meta append)
; => {:doc "concatenates things" :file <fs>/history :line 1 :column 0 :deprecated "Use (concat) instead."}

I think adding that metadata as part of this PR is probably a good idea just to start tracking it. We could/should also change (doc append) to print a deprecation warning if it sees the :deprecated metadata. Right now it only prints the :doc.

To meet your suggestion, we could emit a warning the first time we see a deprecation for a given definition :file and :line. The trick would be doing that without adding a performance penalty on every non-deprecated function call. 🤔 (No need to tackle this as part of this PR!)

Should (append) be updated to use (concat)'s implementation, or is it better to keep it as a duplicate because it's short.

(def append concat) as above should be fine - it'll keep the existing docs, since docs are attached to value itself, not the binding.

[pfiaux]

Updated as you suggested, I still need to figure out how to run the CI and check what building the doc looks like then I'll take it out of draft.

For the warning I was thinking if instead defining append based on concat it's implementation was (with the limits of my current lisp skills) in the lines of:

(def append
  (deprecation "Use (concat) instead.")
  (concat args_here)
)

Then the cost of checking if the warning is already printed would only be paid by the deprecated function not the new implementation. But as you said it might be stretching it for this PR.

[pfiaux]

I was trying to run the pr checks (see also #254) but nothing was happening, does the runner have to be on before I push the commit?

Also when I build the docs I still see append under stdlib (similar to https://bass-lang.org/stdlib.html#binding-append) am I missing something for concat to show up instead?

[vito]

Yep, the runner has to be up and running before pushing at the moment. (TODO) I usually just amend/rebase + push.

Also when I build the docs I still see append under stdlib (similar to https://bass-lang.org/stdlib.html#binding-append) am I missing something for concat to show up instead?

append will still show up since it's still defined. concat should show up too, so I'd expect the only issue is duplication.

It would be great to have the docs mark append as deprecated. I can put work into that soon (or you can if you're curious).

[vito]

Woops forgot to reply to this:

For the warning I was thinking if instead defining append based on concat it's implementation was (with the limits of my current lisp skills) in the lines of: (...)

If I understand correctly, you mean something like this?

(defn append args
  (log "(append) is deprecated; use (concat) instead")
  (apply concat args))

The advantage of the static metadata approach is we can use it for the docs, which we wouldn't be able to do with regular code that evaluates when the function is called. We can also probably find a way to emit a warning based on that metadata being present, and also avoid spamming the logs multiple times for the same call site.

I think at least having it marked deprecated in the docs is a good first step so that's why I prefer having the metadata.

[pfiaux]

I have the runner open currently (as of 16:00 CEST) but I don't see the jobs as started above, from looking at other PRs i'd expect they should show something different if they started running.
Made sure to first start the agent then push a commit so it would pick up the hook, but I get the feeling that didn't work 🤔.

Also I re-ran bass bass/docs and can't seem to see concat still. Also puzzling to me is the code example shows as => (append [1] [2 3] [4 5 6]), I would have expected if duplicated it would say concat for both. Not sure if I'm not running a clean build or if I'm missing something else.

[vito]

@pfiaux I think builds not running is my fault - it's receiving a pull_request.synchronize event but I disabled running builds for that because it was proving redundant with push, but that only affects me since I'm pushing directly to the repo.

https://loop.bass-lang.org/runs/7d2f6c14-14e7-4960-bb11-efaa9c8f4918

Also I re-ran bass bass/docs and can't seem to see concat still. Also puzzling to me is the code example shows as => (append [1] [2 3] [4 5 6]), I would have expected if duplicated it would say concat for both. Not sure if I'm not running a clean build or if I'm missing something else.

Might be better to run ./docs/scripts/booklit -s 3000 and then browse to http://localhost:3000. bass/docs just does a one-time static build and emits it to stdout for piping to bass -e.

[vito]

Pushed a fix, if you want to try pushing again.

[vito]

Merging, and I'll start on the docs change now. Thanks again!

[vito]

(append) is now marked deprecated in the docs:

• labelled deprecated in the index
• deep-link shows deprecation instructions
• docs now support [reference] syntax which gets turned into a nice binding link

...though now the docs are ahead of what's shipped. oops. time to release. :P