greghendershott / rackjure

Provide a few Clojure-inspired ideas in Racket. Where Racket and Clojure conflict, prefer Racket.
BSD 2-Clause "Simplified" License
236 stars 17 forks source link

Add {if,when}-not #12

Closed qerub closed 11 years ago

greghendershott commented 11 years ago

Thanks for the PR! However Travis CI says raco test isn't succeeding, is that something you can get to pass?

greghendershott commented 11 years ago

I pulled the branch and tried locally, and got the same error.

I haven't tried to write this kind of unit test for a macro in Racket before. And I didn't give this a huge amount of thought. But I think what you want instead is:

=>

(module+ test
  (require rackunit)
  (define (check-expansion input expected-output)
    (check-equal? (syntax->datum (expand-once input))
                  (syntax->datum expected-output)))
  (check-expansion #'(if-let [x #t] 0 1)   #'(let [(x #t)] (if x 0 1)))
  (check-expansion #'(when-let [x #t] 0 1) #'(let [(x #t)] (when x 0 1)))
  (check-expansion #'(if-not #t 0 1)       #'(if (not #t) 0 1))
  (check-expansion #'(when-not #t 0 1)     #'(when (not #t) 0 1)))

Does that make sense and work for you, too? If so, could you please either update your PR, or, let me know if you'd like me to add this as a commit on my end?

Thanks again.

qerub commented 11 years ago

I only tested the code in DrRacket. It surprises me that the behavior is different there…

Yes, your suggestion is excellent and I've pushed an updated commit.

greghendershott commented 11 years ago

I'm surprised, too, and asked about that on the Racket mailing list.

qerub commented 11 years ago

It's also interesting that build 1/3 failed on https://travis-ci.org/greghendershott/rackjure/builds/10634283 with the updated commit.

greghendershott commented 11 years ago

That build is using Racket 5.3, which is very old.

Perhaps unnecessarily old: As a result of https://github.com/greghendershott/rackjure/issues/5 I wanted to support versions as old as 5.3.2. But doesn't have to be 5.3.0. Probably I should change that Travis CI build to use 5.3.2 instead of 5.3.0. And if your code passes under 5.3.2, then OK. :)

But I'm also curious to hear back from the mailing list and understand the underlying issue. So although I don't want to leave your PR in limbo, I think I'll give it another 24 hours or so before deciding exactly what to do. If you don't mind?

qerub commented 11 years ago

I'm also curious about the issue and in absolutely no hurry with the patch.

greghendershott commented 11 years ago

It looks like explicitly setting current-namespace is what's required to make it work under 5.3.2.