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

Implement juxt #5

Merged
merged 5 commits into from
May 13, 2023
Merged

Implement juxt #5

merged 5 commits into from
May 13, 2023

Conversation

anonimitoraf
Copy link
Contributor

@anonimitoraf anonimitoraf commented May 4, 2023

Fun project!

I've added an implementation for juxt. I'm a bit inexperienced with macros so completely open to feedback

Basic examples I tested with:

((lambda
   (&rest xs)
   (mapcar
     (lambda (f) (apply f xs))
     '(- +)))
  1 2) ;; => (-1 3)

((lambda
   (&rest xs)
   (mapcar
     (lambda (f) (apply f xs))
     '(1+ 1-)))
  1) ;; => (2 0)

@anonimitoraf
Copy link
Contributor Author

Actually, how do I go about testing this E2E?

@borkdude
Copy link
Owner

borkdude commented May 5, 2023

There's currently no tests. If you feel like working on that, that could be very useful to this project.

For a possible test setup you could look at:

https://github.com/borkdude/flycheck-clj-kondo

Instead of emitting an inline definition of juxt perhaps we could emit a top level function which is then re-used?

@anonimitoraf
Copy link
Contributor Author

anonimitoraf commented May 5, 2023

Thanks, I'll check out clj-kondo.

When you say top-level fn, do you mean implementing defn?

@borkdude
Copy link
Owner

borkdude commented May 5, 2023

I mean, we can emit this first:

(defun juxt ...)

and then we just leave the call to juxt in where it was

@anonimitoraf
Copy link
Contributor Author

anonimitoraf commented May 5, 2023

Sorry, not sure I follow. Clarifying, did you mean to generate:

(defun juxt (&rest fns)
   (&rest xs)
   (mapcar
     (lambda (f) (apply f xs))
     fns))

and reuse this fn for subsequent calls?

@borkdude
Copy link
Owner

borkdude commented May 5, 2023

exactly :)

@anonimitoraf
Copy link
Contributor Author

What's the best way to check if juxt is already defined (so that it could be reused?)

@borkdude
Copy link
Owner

borkdude commented May 6, 2023

We can keep that as state in the global context. But feel free to skip this requirement and continue with the inline implementation, I can take care of that part :)

@borkdude
Copy link
Owner

borkdude commented May 8, 2023

If you're trying to install babashka in Github actions, I recommend using the https://github.com/DeLaGuardo/setup-clojure action. It's much faster than brew.

@borkdude
Copy link
Owner

borkdude commented May 8, 2023

Similarly for Windows: there is no need to use scoop: setup-clojure works on all OSes.

@anonimitoraf
Copy link
Contributor Author

I keep getting this error from eldev:

               signal(void-variable (/bin/sh:))
               apply(signal (void-variable (/bin/sh:)))
               (setq value-257 (apply fn-255 args-256))
               (unwind-protect (setq value-257 (apply fn-255 args-256)) (setq form-description-259 (nconc (list '(should (equal (clj2el-clj! ...) '...))) (list :form (cons fn-255 args-256)) (if (eql value-257 'ert-form-evaluation-aborted-258) nil (list :value value-257)) (let ((-explainer- (and (symbolp ...) (get ... ...)))) (if -explainer- (progn (list :explanation (apply -explainer- args-256))))))) (ert--signal-should-execution form-description-259))

Any ideas?

@borkdude
Copy link
Owner

Doesn't ring a bell. Maybe we can split up the PR to get CI working from the PR that adds juxt?

@borkdude
Copy link
Owner

No never mind, the juxt change seems small enough to this together. Perhaps you can compare what happens in the flycheck-clj-kondo CI compared to here? I'll re-trigger its CI to see if everything is still working over there.

@borkdude
Copy link
Owner

borkdude commented May 12, 2023

Just checked and flycheck-clj-kondo runs fine: borkdude/flycheck-clj-kondo@4c7a6c4

Perhaps @ikappaki has some ideas why this PR is failing

@anonimitoraf
Copy link
Contributor Author

Thanks for the info @borkdude . Upon close inspection, it may have been because I forgot to install clj2el as an "executable".

Now that I do (via bbin), I get this error:

----- Error --------------------------------------------------------------------
Type:     java.lang.Exception
Message:  Could not find namespace: clojure.spec.alpha.
Location: /usr/local/bin/bbin:32:3

----- Context ------------------------------------------------------------------
28:   (upgrade [script])
29:   (uninstall [script]))
30: 
31: (ns babashka.bbin.specs
32:   (:require [clojure.spec.alpha :as s]))
      ^--- Could not find namespace: clojure.spec.alpha.

@borkdude
Copy link
Owner

Try to use a newer version of babashka

@anonimitoraf
Copy link
Contributor Author

Cool, CI passes now. I do have one question though, in a case like this:

(clj2el-clj! (let [fns (juxt + -)]
                  (funcall fns 0 1 2 3 4 5)))

funcall is required (in elisp). Is there a way to get clj2el to insert funcalls when needed?

@borkdude
Copy link
Owner

Great!

Yes, I think we keep track if something is a local, so whenever a local is called, we can insert a funcall

@anonimitoraf
Copy link
Contributor Author

Actually, I think we also need to cover the case of something like:

(def fns (juxt ...))

So on second thought, could we just add a funcall for every fn call?

@borkdude
Copy link
Owner

Let's take it issue by issue. Please wrap up the work that this issue was originally about and then we can address the next problem in a separate issue, else it's getting unwieldy :)

@anonimitoraf
Copy link
Contributor Author

Ah yep, will do

Fix step name

Try manual install

Try installing via brew (for linux/mac)

Set up scoop

Disable windows for now

Add missing "

Attempt to fix test step

Check if /bin/sh exists

Revert "Check if /bin/sh exists"

This reverts commit 0a076bb.

Use clojure Github action instead

Try eldev without packaged mode

Try eldev github action

Find sh executable (so I can symlink it to /bin/sh)

Symlink sh to /bin/sh

Remove test CI for now

Revert "Remove test CI for now"

This reverts commit d8fde84.

Install bbin and clj2el as executable. Only Mac for now

Try latest babashka

Install eldev manually

Attempt to fix bbin setup

Allow me to debug runner via ssh

Debug again

Another attempt

Install bbin manually

Remove unused step

Re-add ubuntu

Re-add windows

Remove windows for now
@anonimitoraf
Copy link
Contributor Author

anonimitoraf commented May 13, 2023

Ok, I've removed windows from the OS matrix for now (having trouble getting bbin setup).

I think the rest of the stuff's ready for review

@borkdude borkdude merged commit 9b1a7c1 into borkdude:main May 13, 2023
@borkdude
Copy link
Owner

@anonimitoraf Thanks a lot, excellent first PR!

@ikappaki
Copy link
Contributor

Just checked and flycheck-clj-kondo runs fine: borkdude/flycheck-clj-kondo@4c7a6c4

Perhaps @ikappaki has some ideas why this PR is failing

Hi, I've addressed the ms-win failrue with #6, thanks.

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

Successfully merging this pull request may close these issues.

3 participants