code-reviews 2020-12-11

@roelof has joined the channel

a small thing: :false and :true should probably be false and true - then your macro can just be (if logging-enabled (prn ~message))`

the nil is implicit, and message needs to be unquoted

in the style of your previous macro, (prn ~message)` could also be (list prn message)` the two are equivalent

@noisesmith thanks, I see now only a the message (prn message) and not (prn :hi) as the challenge said the outcome should be

and a message that the else branch is missing

wait, who says an else branch is missing?

I disagree with clj-kondo, but it's clj-kondo (a style tool) telling you this

Use when instead of if when you need to return nil from else branch.

this is a controversial suggestion - many think that when is implicitly for side effects (I could go either way, but wouldn't go far as saying all one armed ifs should be replaced with when - if allows a single arm for a reason)

I have never seen using when just for side effects. I just use when instead of (if test branch nil)

Or maybe I just have got used to it because I use kibithttps://github.com/jonase/kibit

I usually use (if test branch) or (and test then) for values, I definitely never use (if test branch nil)

When I see (if test branch) I have feeling that the author forgot to add else 😄

it's harmless to add a nil else branch, but it is also redundant

(ins)user=> (if false :OK)
nil
(ins)user=> (if false :OK nil)
nil

it runs so I do not have a problem with it

the only thing that worries me that I see prn message as output instead of (pnr :hi) as stated in the challenge

your original will show that:

(defmacro log [message]
  (if (= logging-enabled :true)
    `(prn message)
    nil))
my version won't:
(defmacro log [message]
  (if logging-enabled
   `(prn ~message)))
I think you skipped part of my feedback

notice the ~ to unquote message

change the if to a when to mollify clj-kondo

or and if you think when is only for side effects and don't mind a random false instead of nil :D

@noisesmith then I see (prn ~ message) as output

are you sure you aren't using ' instead of ` ?

the ~ should not show up in the macro expand, it's a special instruction to the ` reader macro

(def logging-enabled true)

(defmacro log [message]
  (if logging-enabled
    '(prn ~message)))


(macroexpand `(log :hi))

that's not `, it's '

oke, this code works

thanks for the feedback