Core.async integration doc-string

[jarohen]

Originally posted by @jimpil in https://github.com/jarohen/chime/issues/37#issuecomment-753000735

Re: the core-async integration:

At first glance, I don't see how the stuff we're discussing breaks chime-ch. However, I did notice something else. The doc-string states:

ch - (optional) Channel to chime on - defaults to a new unbuffered channel Closing this channel stops the schedule.

I see no evidence of this statement. I suspect you meant to say closing the returned channel stops the schedule, but then that should be a few lines up. If the intention of the doc-string is correct, then I think the chiming-fn should be something like the following:

(fn [time]
  (when-not (a/>!! ch time) 
    (a/close ch)))

With the risk of rushing to conclusions, i will say that this looks like another remnant of a time when chime would actually consider the return of the chime-fn. However, at the moment it is not (true is returned after it). I apologise in advance if that's far-reaching...

[jimpil]

Here is an implementation of chime-ch that honors the doc-string (and if you let the user provide the channel, you pretty much have to honor it):

(let [ret (promise)
        sched (chime/chime-at times
                              (fn [time]
                                (when-not (a/>!! ch time)
                                  (a/close! @ret)))
                              {:on-finished (fn [] (a/close! @ret))})]
    (->> (reify
           p/ReadPort
           (take! [_ handler]
             (p/take! ch handler))

           p/Channel
           (close! [_] (.close sched) (a/close! ch))
           (closed? [_] (p/closed? ch)))

         (deliver ret)
         deref))

[jimpil]

There is still the question of what happens in case of an error. The schedule will stop, but both the returned channel, and the ch param will remain open when relying on the default error-handler, and there is no option to provide a custom one.

(defn chime-ch
  "..."
  [times & [{:keys [ch error-handler on-finished], :or {ch (a/chan)}}]]

  (let [ret (promise)
        sched (chime/chime-at times
                              (fn [time]
                                (when-not (a/>!! ch time)
                                  (a/close! @ret)))
                              {:on-finished (fn [] 
                                              (a/close! @ret) 
                                              (and on-finished (on-finished)))
                               :error-handler (fn [e] 
                                                (if error-handler 
                                                  (error-handler e) ;; user has the option to close `ch` here
                                                  true))})]
    (->> (reify
           p/ReadPort
           (take! [_ handler]
             (p/take! ch handler))

           p/Channel
           (close! [_] (.close sched) (a/close! ch))
           (closed? [_] (p/closed? ch)))

         (deliver ret)
         deref)))

[jimpil]

First of all happy new year!!! All the best wishes to you and your family :)

Secondly, I took a step back and wondered...

what is the point of letting the user provide the underlying channel?

After all, chime-at returns a readable channel - why would we let the user provide the underlying write channel too? The only reason I can think of is buffering (i.e. perhaps the user wants to supply a buffered channel). In light of this consider the following alternative implementation:

(defn chime-ch
  "..."
  [times & [{:keys [buffer error-handler on-finished]}]]

  (let [ch (a/chan buffer) ;; build the channel internally
        ret-ch (promise)
        error-handler (if error-handler
                        (fn [e]
                          (or (error-handler e)    ;; user's error-handler says to carry on with the schedule
                              (a/close! @ret-ch))) ;; user's error-handler says to stop the schedule
                        (constantly true)) ;; user didn't provided an error-handler - carry on with the schedule
        sched (chime/chime-at times
                              (fn [time] (a/>!! ch time)) ;; nothing special to do here
                              {:on-finished (fn []
                                              (a/close! ch) ;; only this needs closing at this point
                                              (and on-finished (on-finished)))
                               :error-handler error-handler})]
    (->> (reify
           p/ReadPort
           (take! [_ handler]
             (p/take! ch handler))

           p/Channel
           (close! [_] (.close sched) (a/close! ch)) ;; close both!
           (closed? [_] (p/closed? ch)))

         (deliver ret-ch)
         deref)))

I fully understand you don't like breaking changes, but do you see how simpler/clearer this becomes? Basically you only need to worry about the thing that you give back to the user. Anyway, enough for tonight ;)