improves (map)

[agzam]

Our previous implementation of map apparently would leave around GCiable structures that for some reason would hang around, in rare cases causing memory-leaks.

This bug, I think, usually manifests itself on multi-monitor setups. What I used to observe: for no apparent reason Hammerspoon hangs, and the system would "swallow" certain keys - e.g., "a", "w", "j" - the keys on the main modal. Which consequently makes it difficult to type something like "killall Hammerspoon" in the terminal, and one has to use ActivityMonitor (which would show HS eating up like gigs of mem... crazy) to kill and restart Hammerspoon (using only mouse), even the tray icon wouldn't work.

This refactoring makes the (map) similar to Clojure function, you can feed multiple collections (tables) into it.

Possibly fixes: #186, #164, #77

[jaidetree]

Looks good but haven't tested it. Given map just lightly wrapped reduce, maybe that implementation needs to change to mitigate this issue too?

[jaidetree]

Also, great catch!

[agzam]

maybe that implementation needs to change to mitigate this issue too?

I had no access to a Mac for several months, which forced me to venture into AwesomeWM, which is also Lua-based, and of course I picked it so I could use Fennel. It has a more verbose logging option, and some weird messages with (map), which I initially borrowed from this repo, forced me to reimplement it.

I need to do some diffing and see if I made some other improvements that could also be moved here now. This change incidentally fixed a long-standing personal headache for me. Hammerspoon didn't hang once in the past few days for me.

[jaidetree]

maybe that implementation needs to change to mitigate this issue too?

I had no access to a Mac for several months, which forced me to venture into AwesomeWM, which is also Lua-based, and of course I picked it so I could use Fennel. It has a more verbose logging option, and some weird messages with (map), which I initially borrowed from this repo, forced me to reimplement it.

I need to do some diffing and see if I made some other improvements that could also be moved here now. This change incidentally fixed a long-standing personal headache for me. Hammerspoon didn't hang once in the past few days for me.

Glad it's fixed! Can't say I experienced that but yeah, glad it's resolved. Only behavior I've observed is it eventually crashing on Sonoma. Between the recent hammerspoon release and this fix, that might just fix it.

Btw have you started using WezTerm? Switched to it a bit ago after I found out it's also configured with lua and was able to get fennel there. One nuance though is they didn't implement the debug interface so you'll have to stub out the APIs so macros work as expected.

I can take on improving the reduce implementation and writing tests for it

[agzam]

WezTerm? No, never heard of it. Aww... yet another rabbit-hole :) I'm using Kitty, as my terminal needs lately have been quite unsophisticated. I usually run things inside Emacs. My most recent epiphany is that mpv is also configurable with Lua, and I've been thinking if I need to do some hacking on that. It's an incredibly liberating approach to watch any video content when you can control the video playback directly from your editor - you can pause/resume, speed it up, control volume, etc. Great for note taking, for example. And of course, with Fennel/Lua, you can do the opposite as well - e.g., jump to things in your editor from the playing video.

[Grazfather]

I actually have been plagued with Hammerspoon hangs for some time. Strangely only on one of my two machines.

I just updated to v1.0.0 the other day, though, and haven't been seen a hang yet.

Here's what I'll do:

1. I'll give 1.0.0 a few more days, if it hangs, I'll keep it, if it's good, I'll downgrade to a known-bad version.
2. I'll test out this PR and see if it 'fixes' the issue.

Also, I use wezterm and I am a big fan as well, though my config is super simple and it's still in Lua.

[Grazfather]

Can we add some test cases to test/funstional-test.fnl?

[Grazfather]

do you have to assign the name here or can you just return (f (table.unpack ...))?

[Grazfather]

This seems to 'double nest' it. E.g. if I call (apply add [1 2]) in apply args is { { 1, 2} }

[agzam]

I don't think (f (table.unpack ...)) works.

if I call (apply add [1 2])

You're giving it a single argument. (apply) usually works with multiple, in your case you'd call it (apply add 1 2)

[Grazfather]

what's the point of apply, then? Couldn't you just call (add 1 2)?. As I understand it (from clojure), (apply add [1 2])) should be equivalent to (add 1 2)

[Grazfather]

Have you given this one more thought?

[Grazfather]

And I figure we should probably work in the same way as clojure

[agzam]

Let's put a pin on this one. It definitely requires some more thought. Even fennel-cljlib implementation is not so trivial.
We need to handle cases like:

user> (apply print [1 2 3])
1 2 3;; => nil
user> (apply print 1 [2 3])
1 2 3;; => nil
user> (apply print [1 2] [3 4])
[1 2] 3 4;; => nil

Why doesn't Clojure unpack both vectors in the last case I have no idea, but that's how it seems to work.
Seven peduncles, I had no idea how Clojure's (apply) is unpredictable, lol:

user> (apply print [:one :two :three])
:one :two :three;; => nil

user> (apply print [:one :two :three] [:four])
[:one :two :three] :four;; => nil

user> (apply print :one :two :three)
java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Keyword 

(apply print [:one :two :three] :four)
java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Keyword 

[Grazfather]

oh that's strange. I don't think I have tried to use apply with more than 2 args (a function and a sequence).

I am ok with figuring this out later. We can remove this and merge the rest?

[agzam]

hmmm... apply is used though. in (map) particularly. I'd say let's merge it as is and then maybe improve it later? I think for general use cases it works. Or do you prefer to get it right instead and not to increase the tech-debt budget?

[Grazfather]

Seems that clojure basically only unpack the last argument, which must be a sequence. This implementation collects all args into a table and then unpacks that, which is also incorrect.

map only uses apply with two args, so what I think might be best would be to write this apply for 2 args [fn seq] and unpack that seq, we can later add support for extra intermediate non-splices args between the function and the last sequence.

[jaidetree]

Going to block until after these issues are at least discussed

[Grazfather]

I think we should merge after #194 and #195, and add tests for the new functions. #195 adds a test for concat, which is changed (but then overwritten) in this PR.

[agzam]

I did some rebasing and addressed the comments, give it another gander, folks? @Grazfather @jaidetree

[jaidetree]

I did some rebasing and addressed the comments, give it another gander, folks? @Grazfather @jaidetree

Thanks Ag! @Grazfather could you please test it? My config is operating off of the spoon branch + tweaks for nix and is very fragile right now

[Grazfather]

Tests pass, have a few Qs still though.

Sorry for the delay, was out of town.