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

cap10morgan · 2024-03-15T16:55:32Z

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 · 2024-03-16T14:12:54Z

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 · 2024-03-18T17:34:49Z

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

cap10morgan · 2024-04-02T20:23:46Z

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

cap10morgan · 2024-04-16T15:11:32Z

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

cap10morgan · 2024-04-25T15:01:42Z

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

weavejester · 2024-04-25T18:07:25Z

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

weavejester

Thanks for the commits. I have some questions/suggestions. The primary issue to be solved is that the reporting function needs a consistent type signature.

cljfmt/src/cljfmt/tool.clj

weavejester · 2024-04-25T18:13:28Z

cljfmt/src/cljfmt/tool.clj

+(defn check-paths
+  "Runs a check on the provided paths and returns a map detailing the results."
+  [paths & [options]]
+  (let [opts (merge {:report report-lib}
+                    (config/load-config) options
+                    {:paths paths})]
    (check-no-config opts)))


Why add this function? Why not make it equivalent to using report-lib with check?

Well, the entire point of this PR is to make this more ergonomic to use as a library. So this is the library entry point fn for checking paths.

Not sure I understand this:

Why not make it equivalent to using report-lib with check?

I mean something like:

(check {:paths ["src"], :report report-lib})

That is pretty much what it is. It just didn't seem very nice to require library users to manually specify the reporting fn they will always want when a simpler entry point fn can do that for them.

I will also note that this is a big part of why I started with a different namespace. Naming this fn in here is tricky in terms of revealing its intention (and so was naming the other namespace as you probably recall; so that's perhaps not a great solution either).

But the idea is you would call this fn when using cljfmt as a library to check paths (instead of strings), and the existing fn(s) are still for internal CLI usage. I'm open to ideas about how to better name this, document that intention, etc.

But I do think it makes sense to have a "library entrypoint" fn that doesn't require setting options you'll want every time to make it be a well-behaved library (e.g. not calling System/exit, not printing to stdout, etc.).

I think I'd rather omit the check-paths and fix-paths wrapper functions for now. I don't think it's a huge deal to add an extra :report argument to cater for a relatively niche use-case.

I think it would also be a good idea to move report-cli to cljfmt.report/console and report-lib to cljfmt.report/clojure. However, don't feel you need to make that chage; I can always move things around after the PR is merged.

OK, I can make both changes

Would you prefer these changes as new, separate commits? Or should I just continue to amend the existing HEAD commit on this branch?

I'll go ahead and push separate commits for easier review. Happy to squash later if desired.

When I moved the reporting fns to the new cljfmt.report ns, it required one other change to avoid a circular dependency: The project-path fn is used by both cljfmt.tool fns and cljfmt.report fns, so it needed to be in a namespace they both required. cljfmt.io seemed like a natural fit, so I moved it there.

cljfmt/src/cljfmt/tool.clj

weavejester · 2024-04-25T18:15:35Z

cljfmt/src/cljfmt/tool.clj

-                           (print-file-status options status)
-                           (:counts status)))
-                    (reduce merge-counts))]
+(defn- merge-statuses


I don't understand the purpose of merge-statuses. Can you explain?

It's the counterpart to merge-counts when you want a fuller report of what was done, which you often do when using this as a library.

cljfmt/src/cljfmt/tool.clj

weavejester · 2024-04-25T19:33:19Z

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 · 2024-04-25T20:24:55Z

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

cljfmt/src/cljfmt/tool.clj

weavejester · 2024-04-26T17:52:43Z

cljfmt/src/cljfmt/tool.clj

+(defn- merge-statuses [a b]
+  (let [^File file (:file b)
+        path       (.getPath file)
+        diff       (:diff b)
+        exception  (:exception b)]
+    (cond-> a
+            true (update :counts #(merge-with + % (:counts b)))
+
+            (and file (= 1 (-> b :counts :incorrect)))
+            (assoc-in [:incorrect path] "has incorrect formatting")
+
+            (and file diff) (assoc-in [:incorrect path] diff)
+            (and file exception) (assoc-in [:error path] exception))))


Maybe something more like:

(defn- merge-statuses [results {:keys [file diff exception counts]}] (let [path (some-> file .getPath)] (-> results (update :counts merge-counts counts) (cond-> (and path (> (:incorrect counts) 1)) (assoc-in [:incorrect-files path] {})) (cond-> (and path diff) (assoc-in [:incorrect-files path :diff] diff)) (cond-> (and path exception) (assoc-in [:errored-files path :exception] exception)))))

cap10morgan · 2024-04-29T16:20:34Z

@weavejester Should be ready for review again

weavejester

Thanks for all your work! I just have one query this time.

weavejester · 2024-05-04T12:58:32Z

cljfmt/src/cljfmt/tool.clj


 (defn check
  "Checks that the Clojure paths specified by the :paths option are
  correctly formatted."
  [options]
-  (let [opts (merge (config/load-config) options)]
+  (let [opts (merge {:report report/console}


Is the merge of {:report report/console} necessary given the :or set in check-no-config?

And similarly for fix and fix-no-config?

Good catch. No they are not! 😁 Will fix shortly

Fixed in ddf28c0

weavejester · 2024-05-10T21:07:52Z

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

cap10morgan · 2024-05-11T15:15:44Z

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 · 2024-05-16T01:09:44Z

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 · 2024-05-16T14:47:39Z

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 · 2024-05-20T16:15:48Z

@weavejester 69ca107 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 · 2024-05-25T22:42:45Z

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.

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

cap10morgan · 2024-05-28T15:28:12Z

@weavejester done

cap10morgan · 2024-06-03T15:51:24Z

@weavejester Anything else needed here before merging?

weavejester · 2024-06-07T13:53:00Z

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.

cap10morgan force-pushed the feature/reporting-fn branch from bec341e to ec87e5e Compare April 2, 2024 20:05

weavejester requested changes Apr 25, 2024

View reviewed changes

cap10morgan force-pushed the feature/reporting-fn branch from ec87e5e to 5ee10c0 Compare April 25, 2024 20:24

cap10morgan force-pushed the feature/reporting-fn branch 2 times, most recently from 0c8acc9 to b6b7966 Compare April 25, 2024 20:28

weavejester reviewed Apr 26, 2024

View reviewed changes

cljfmt/src/cljfmt/tool.clj Outdated Show resolved Hide resolved

cap10morgan force-pushed the feature/reporting-fn branch from b6b7966 to 3133e0e Compare April 26, 2024 17:34

weavejester reviewed Apr 26, 2024

View reviewed changes

cap10morgan force-pushed the feature/reporting-fn branch from 9c4e16f to a11f930 Compare April 29, 2024 16:17

weavejester reviewed May 4, 2024

View reviewed changes

cap10morgan force-pushed the feature/reporting-fn branch from ddf28c0 to 3211f99 Compare May 11, 2024 15:13

cap10morgan force-pushed the feature/reporting-fn branch from 3211f99 to 5e38c3c Compare May 16, 2024 14:47

Add :report option to check and fix

d6bab1d

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

cap10morgan force-pushed the feature/reporting-fn branch from 69ca107 to d6bab1d Compare May 28, 2024 15:28

weavejester merged commit 8d8e26d into weavejester:master Jun 9, 2024
1 check passed

cap10morgan mentioned this pull request Jun 19, 2024

Add fix bb task to run cljfmt fix on code fluree/server#58

Merged

cap10morgan deleted the feature/reporting-fn branch June 19, 2024 16:45

cap10morgan mentioned this pull request Jun 19, 2024

Use hopefully soon-to-be-official cljfmt lib entrypoint fluree/server#57

Merged

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

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

Conversation

cap10morgan commented Mar 15, 2024

weavejester commented Mar 16, 2024

cap10morgan commented Mar 18, 2024

cap10morgan commented Apr 2, 2024

cap10morgan commented Apr 16, 2024

cap10morgan commented Apr 25, 2024

weavejester commented Apr 25, 2024

weavejester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weavejester commented Apr 25, 2024

cap10morgan commented Apr 25, 2024

Choose a reason for hiding this comment

cap10morgan commented Apr 29, 2024

weavejester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weavejester commented May 10, 2024

cap10morgan commented May 11, 2024

weavejester commented May 16, 2024

cap10morgan commented May 16, 2024

cap10morgan commented May 20, 2024

weavejester commented May 25, 2024

cap10morgan commented May 28, 2024

cap10morgan commented Jun 3, 2024

weavejester commented Jun 7, 2024