Feature: reporting fn to allow more ergonomic use as a library

[cap10morgan]

This is a follow-up to #333 with the implementation outlined by @weavejester over there.

I think the main decision I made in here was the name and signature of the "library entry point" fns. I went with check-paths and fix-paths that take a paths collection as a first arg and then an optional options map. I did this b/c the primary purpose of this refactor was to create library-friendly entry points that check / fix paths like the CLI tool entry points do but w/o doing things like printing to stdout or exiting the whole process.

But I don't have strong opinions about any of that, so let me know if you'd like to see any changes there or anywhere else.

[weavejester]

Thanks for the PR, and my apologies for not being clear about the design I was suggesting. In clojure.test and other similar libraries, the reporting function reacts to events like :pass, :fail and :summary, and then decides how it will output these. We could do something similar with cljfmt. Consider a multimethod:

(defmulti cli-report
  (fn [event context data] event))

Used like:

(defn check-no-config [{:keys [parallel? paths report] :as options}]
  (let [map*    (if parallel? pmap map)
        summary (->> paths
                     (mapcat (partial find-files options))
                     (map* (partial check-one options))
                     (reduce #(report :check/file %1 %2) options))]
    (report :check/summary summary nil)))

The reporter would be responsible for reporting data for the user. This includes both outputting on a file-by-file basis, and collating the data in a context map for use in the summary. We could use the options as the initial context, and add a ::count key to it.

I'd imagine we'd want at least 4 events: :check/file, :check/summary, :fix/file and :fix/summary. Maybe a :check/initial and :fix/initial as well that would be no-op by default, but available for reporters that want to setup something like a progress bar.

[cap10morgan]

@weavejester Makes sense. I'll work on moving it in that direction next chance I get. Thanks for the feedback!

[cap10morgan]

@weavejester this should now be working w/ the event-based approach you outlined.

[cap10morgan]

@weavejester Any other TODO's here? Happy to refine if this needs some more work before merging.

[cap10morgan]

@weavejester just checking in again to see where we're at with this PR

[weavejester]

Apologies for the delay in looking at this. I'll review the code now.

[weavejester]

To give a little more context, this was the design I had in mind for the CLI reporter. Note that the method signature remains consistent: event context data -> context, with the initial context being the options map.

(defmulti report-cli
  (fn [event _context _data] event))

(defmethod report-cli :check/init [_ context _] context)

(defmethod report-cli :fix/init [_ context _] context)

(defmethod report-cli :check/file [_ context file-status]
  (print-file-status context file-status)
  (update context :counts (fnil merge-counts {}) (:counts file-status)))

(defmethod report-cli :fix/file [_ context file-status]
  (print-file-status context file-status)
  context)

(defmethod report-cli :check/summary [_ {:keys [counts]} _]
  (print-final-count counts)
  (exit counts))

(defmethod report-cli :fix/summary [_ _ _])

[cap10morgan]

OK @weavejester I just pushed some changes that I think address your feedback

[cap10morgan]

@weavejester Should be ready for review again

[weavejester]

Thanks for your work on this. It all looks good. Can you squash your commits down and give it an appropriate commit message?

[cap10morgan]

Thanks for your work on this. It all looks good. Can you squash your commits down and give it an appropriate commit message?

Done

[weavejester]

Thanks! Could you change the commit message to:

Add :report option to check and fix

Allows for a custom reporter function when checking/fixing files.

I think that's perhaps a little clearer.

[cap10morgan]

Thanks! Could you change the commit message to:

Add :report option to check and fix

Allows for a custom reporter function when checking/fixing files.

I think that's perhaps a little clearer.

done

[cap10morgan]

@weavejester https://github.com/weavejester/cljfmt/pull/342/commits/69ca1078a91d5f6222b5feca3d32642a7f21d7bf fixes the CI issue. It was flagging that I hadn't wired up the tool/exit fn into the new event-based workflow. It should now mimic previous behavior. Let me know if that looks good and I'll re-squash.

[weavejester]

Thanks for the fix, and sorry for the delay again. It's been a busy week! It all looks good if you want to squash the changes.

[cap10morgan]

@weavejester done

[cap10morgan]

@weavejester Anything else needed here before merging?

[weavejester]

Sorry for the delay! I've been away for a couple of weeks. I don't think there's anything left to do, so I'll merge once the checks are done, but I may delay the release a little as I think about the format of the reporters.