diff --git a/doc/arch/adr-003.md b/doc/arch/adr-003.md new file mode 100644 index 00000000..5e3ce5ca --- /dev/null +++ b/doc/arch/adr-003.md @@ -0,0 +1,189 @@ +# ADR 003: Easier editing of error messages + +## Problem Statement + +Besides a fixed set of [printer options](https://github.com/bhb/expound#printer-options), users can only manipulate error messages by parsing and manipulating the string returned by `expound-str`. + +Expound does not consider the format of the error messages to be part of the API and so changes that break client string-parsing code is likely to break at some point. + +Why might a user want to manipulate the error message? + +* Minimizing [large vectors](https://github.com/bhb/expound/issues/108) or [maps](https://github.com/bhb/expound/issues/129) when printing out the invalid value +* [Adding additional information](https://github.com/bhb/expound/issues/148) + +### Specific problems + +#### [#129](https://github.com/bhb/expound/issues/129) + +* When a map matches some user-defined pattern (e.g. contains some keys) and/or when the problem is of a certain problem type (e.g. the "missing keys" type, which currently shows a table of missing keys), hide most of the map +* Note that for some problem type, this could hide info - e.g. a predicate that checked length of map +* Some information about the value may not just be used to understand why it failed predicate, but also to locate the value. For instance, consider an array with many maps, in which every map contains an `:id` key - we may not want to hide the `:id` value if it helps user locate the invalid value. + +#### [#110](https://github.com/bhb/expound/issues/110) + +* When sequences are long, shrink output by omitting repeated "..." values +* This depends on value, not necessarily on type of problem +* For certain values with many values that are similar, only showing the failing value is insufficient, but `print-valid-values` shows too much data. +* Perhaps whitelist values to show? +* Perhaps add notion of "distance" from problem? Does this depend entirely on type of data? + +#### [#108](https://github.com/bhb/expound/issues/108) + +* When value is very large (especially nested values), hide higher levels of values +* Does not generally depend on problem type + +#### [#148](https://github.com/bhb/expound/issues/148) + +* Given the values and the error, allow user to inject content between value and the error (or really, anywhere) + +#### "Embed spec errors" (No issue filed) + +> My case: user creates a router, a lot of rules are run (syntax, conflicts, dependencies, route data specs etc.). Spec validation is just one part and… I would like to return enought context information what and where it failed. Currently: + +``` +(require '[reitit.core :as r]) +(require '[clojure.spec.alpha :as s]) +(require '[expound.alpha :as e]) + +(s/def ::role #{:admin :manager}) +(s/def ::roles (s/coll-of ::role :into #{})) + +(r/router + ["/api" {:handler identity + ::roles #{:adminz}}] + {::rs/explain e/expound-str + :validate rs/validate-spec!}) +; CompilerException clojure.lang.ExceptionInfo: Invalid route data: +; +; -- On route ----------------------- +; +; "/api" +; +; -- Spec failed -------------------- +; +; {:handler ..., :user/roles #{:adminz}} +; ^^^^^^^ +; +; should be one of: `:admin`,`:manager` +; +``` + +Given some context, add additional context around the spec. + +#### Customize printing for record or value (No issue filed) + +> Can I prevent Expound from printing a certain record? It generates something like 140k lines of output which is not helpful at all. I defined a `print-method` so that prints something simple with `prn` but that does not seem to affect Expound. + +Given some value (or properties of value), allow user to specify custom output (presumably to shrink size of output). + +### Non-problems + +* Minor formatting tweaks to error messages e.g. changes in indentation +* No requests to change the specific characters i.e. hide values with ",,," instead of "." or underline with "---" instead of "^^^" + +## Context + +Currently Expound can only return a single string for the error message. If a user wants to add, omit, or change information, they must do so using the string. + +It's not clear that users would use an solution that allowed for easier editing of error messages: they might instead just want more fixes or features that solve the problem automatically. + +If Expound tries to do the right thing in all cases, it will get complex to maintain and to use since users will have to guess how/why it's showing or omitting certain information. + +## Prior Art + +### [Hiccup](https://github.com/weavejester/hiccup) + +Hiccup is a data format for representing HTML + +**Tradeoffs in the context of this problem:** + +* \+ Optional second element (map) is a place to add classes and IDs, which add semantic meaning +* \- Not intended to cover aspects like indentation and whitespace, since HTML/CSS is responsible for that + +### [Fipp](https://github.com/brandonbloom/fipp/blob/master/doc/primitives.md)
 +Fipp is a better pretty printer for Clojure and ClojureScript + +**Tradeoffs in the context of this problem:** + +* \+ Focused on pretty printing, so primitives reflect that +* \- No place to add meaning which would allow users to find content based on ID or class +* ? Potentially more general than this particular problem requires +* ? Fipp is very fast, but Expound doesn't require that much speed + +### [Clojure pretty printer](https://clojure.github.io/clojure/doc/clojure/pprint/PrettyPrinting.html) + +Expound currently uses Clojure's built-in pretty-print to display values. + +> More advanced formats, including formats that don't look like Clojure data at all like XML and JSON, can be rendered by creating custom dispatch functions. + +- + +Flowchart of how to use + +* \- Need slightly different implementation for ClojureScript and Clojure +* \- Seems to only work on types, need a new type for each role in data structure +* \+ Wouldn't have to worry about writing layout engine +* \+ Configurable by users if they didn't like how I formatted tree + +## Possible solutions: general approach + +### Include Fipp + +Expound could depend on Fipp for pretty-printing. All functions that generate a string would return Fipp data structures. + +**Tradeoffs** + +* \+ Don't need to reimplement algorithm +* \+ Can rely on existing documentation for data primitives +* ? Does Fipp provide code to extract/manipulate the primitives? Do we want to provide this so Expound clients can manipulate data structure? +* \- Take on two new deps: Fipp and `org.clojure/core.rrb-vector`. + * Fipp is well-maintained, but there are complex issues like [this](https://github.com/brandonbloom/fipp/issues/60) +* \- Doesn't provide way to add semantic information about elements beyond formatting +* \- Requires a wrapping layer if we don't want clients to be exposed to Fipp internals + +### Write a custom data description + +I could come up with my own custom data language to describe the errors. + +* \- Need to write more code +* \+ Can make it custom to error messages +* \- Need to write more documentation for format +* \+ Creates a more stable API for changes (format is stable, although content of strings are not) +* \- Creates a more complex API (more functions) +* \- Requires more testing for backwards compatibility +* \+ Speeds up building internal features +* \+ Provides solution for advanced users that doesn't rely on string manipulation + * Encourages libraries to use Expound as dependency + * Provides temporary solution for advanced use cases (can still build configuration if I want, but not necessary for all cases) + * May provide long-term fix for use cases I don't want to move into Expound + +### Don't expose data description + +I could make an internal data API that is not exposed to users. + +**Tradeoffs** + +* \+ No need to add new functions (which won't be used my most users anyway) +* \+ Still allows faster iteration on internal features +* \- If I want to solve the issues linked above, I'll need to either add complexity to the Expound rules OR add more configuration + * Additional configuration adds potential for bugs as number of combinations grows + * I'm not sure users use the configuration I expose now +* \+ No additional API to test for breaking changes +* \- String format becomes an API that users cannot rely on +* \- Does not encourage tools to use Expound internally + +## Possible solutions: data language + +### Return original value plus mapping from `:in` path to value + +It's too tricky to embed metadata within the original data, so we provide a lookup table that provides information to assist with formatting. + +We also need to pass along the category of failure because we might alter the output. + +The deal-breaker here is that I can't meaningfully traverse the original data structure in order and get relevant metadata. Metadata needs to be included because there's no mechanism to look it up later. + +## Decision + +## Status + +Draft diff --git a/src/expound/alpha.cljc b/src/expound/alpha.cljc index 5de91050..6d1bbd3f 100644 --- a/src/expound/alpha.cljc +++ b/src/expound/alpha.cljc @@ -1120,3 +1120,283 @@ returned an invalid value. "Given a sequence of results from `clojure.spec.test.alpha/check`, returns a string summarizing the results." [check-results] (with-out-str (explain-results check-results))) + +(comment + + ;; value + ["one" "two" 33] + ;; [... ... 33] + ;; ^^ + + ;; maybe a map of paths to meta, including distance + { + [0] {:distance 1 :type :peer} + [1] {:distance 1 :type :peer} + [2] {:distance 0 :type :value} ; distance zero is highlighted value + [] {:distance 2} ; not sure how to compute distance, but it does seem "further away" + } + + + + ;; what could you do here? maybe over values to find distance x + ;; and change it? + + ;; every value is a container, or a key, or a value + ;; I don't think 'peer' is useful becaue it only applies to + ;; elements that are peers of distance 0 value + + ;; Expound - a predicate like nil only needs location vs value. Location is where to find it, value is what it is. + + + + :kw + + ;; { [] {:distance 0, :type :value}} + + ;; need to design own pretty print algorithm to layout values + + + + [:foo 1 :bar :baz] + + + + {:age 1 :keys-spec/name :bob} +;; {:age ..., :keys-spec/name :bob} +;; ^^^^ + + { + ;; or whatever path incantation is necessary for finding keys + ;; are the keys further away than the value? + [:age 0] {:distance 2 :role :key :hidden? false} + [:age 1] {:distance 1 :role :value :hidden? true} + [:keys-spec/name 0] {:distance 1 :role :value, :hidden? true} + [:keys-spec/name 1] {:distance 0 :role :value, :hidden? false, :highlight? true} + ;; Is the containers further distance than the keys? + [] {:distance 3 :role :container} + } + + (write-out {}) + + + clojure.pprint/*print-pretty* + *print-length* + + (println (range 0 30)) + + clojure.pprint/*print-pprint-dispatch* + + + + (binding [clojure.pprint/*print-pretty* true] + (clojure.pprint/with-pretty-writer + + (clojure.pprint/write-out (range 0 30))) + + ) + ;; how dispatching works on multimethod + ;; https://github.com/clojure/clojurescript/blob/23cedecbf4f704f9fee672e395bbfa1e3fe3ee1a/src/main/cljs/cljs/pprint.cljs#L2893-L2909 + ;; vs + ;; https://github.com/clojure/clojure/blob/4ef4b1ed7a2e8bb0aaaacfb0942729252c2c3091/src/clj/clojure/pprint/dispatch.clj#L151-L157 + ;; in CLJ + + ;; example of printing + #_(pprint/pprint-logical-block :prefix "[" :suffix "]" + (pprint/print-length-loop [aseq (seq (:x m))] + (when aseq + (pprint/write-out (first aseq)) + (when (next aseq) + ;; This will have to change for CLJS + #_ (-write *out* " ") + (.write ^java.io.Writer *out* " ") + (pprint/pprint-newline :linear) + (recur (next aseq)))))) + + (require '[clojure.pprint :as pprint]) + + ;; hack to redefine method + (def new-dispatch nil) + + (defmulti new-dispatch + "TODO - name" + (fn [m] + (:role m) + ) + ) + + ;; this is promising but how do I connect each form + ;; with metadata? obviously each form is not unique overall + + ;; since this is breaking change, probably don't release except for in expound.alpha2 + + + ;; the way this used to work is that we walked the tree using 'summary value' and printed it out as + ;; we went, using the through the structure and re-created it recursively + ;; + + ;; this is tricky because naively printing out the form recursively will not + ;; capture the metadata in the data + ;; it's an open question whether we can really reliably create that metadata, + ;; but that's a problem for another day. + + ;; HERE - how to recursively print out data structure and grab meta-data + ;; data strucutre is implicit in structure which is a good thing + ;; additionally if we want to print out index of bad value, (location) + ;; then we need more than the current value + + ;; TODO: transfer these notes to ADR + ;; let's rethink location. + ;; why is showing location as part of data structure useful? + ;; because mapping a path to a data structure omits the container types + ;; [0 0 0 1] doesn't say if those are lists, vecs or (unlikely but possible) + ;; maps so user has to figure that out in their head. + ;; The question we're trying to answer is: where is the invalid value located? + ;; This is fundamentally a function of the context value, the specific value + ;; (need better terminology for this), and also I think, the problem type itself + + ;; context value - the entire value that is checked + ;; problem value - the specific value that is failing + + ;; + ;; Actually the problem of describing the location is arguably NOT related to the + ;; problem type. We always want to consistently show where the problem is, but + ;; depending on the problem type, we might be able to summarize the problem value? + + + ;; Hm, that's not quite true, because there are actually three concerns here. + ;; 1. "context value" - this helps us show location of the "problem value" + ;; 2. "problem value" - help understand problem (by display enough info), also helps identify + ;; problem value (imagine a giant map which should be nil, do we need to print it all? Or + ;; can we summarize it for that predicate?) + ;; so it's printing enough of the problem value in order to understand problem. + ;; Problem value and problem are connected, but + ;; The problem here is that people identify values via location and the problem value itself, + ;; so it's not clear that we can summarize generally, since it may be more useful to say "oh, the thing that has x" + ;; vs "the thing located at spot x" + ;; This is especially relevant for recursive values where things can be nil, do we need to show the entire top-level value + ;; just to say it's nil? + ;; Maybe a summary is sufficient with suffient location info? + ;; If the values in the context are tagged with distance, we could also tag problem values with depth. + + ;; What if we had type, distance, depth. + + ;; 3. problem - separate from above, describes what is wrong + + ;; Are there other ways to show context? + + + (defmethod new-dispatch :default [x] + (pprint/simple-dispatch x) + ) + + ;; This might work well because it won't affect print width + (def marker :expound.problems/relevant) + + (defmethod new-dispatch :value [m] + (cond + (zero? (:distance m)) + (pprint/pprint-logical-block + (pprint/write-out marker) + (pprint/write-out (:x m)) + (pprint/pprint-newline :linear) + (pprint/write-out marker) + (.write ^java.io.Writer *out* " ") + ) + + (:hidden? m) + (pprint/pprint-logical-block + (.write ^java.io.Writer *out* "...") + (.write ^java.io.Writer *out* " ") + (pprint/pprint-newline :linear) + ) + + :else + (pprint/pprint-logical-block + (pprint/write-out (:x m)) + (.write ^java.io.Writer *out* " ") + (pprint/pprint-newline :linear) + ) + + ) + ) + + (defmethod new-dispatch :collection [xs] + (doseq [x (:x xs)] + (pprint/write-out x) + ) + ) + + + (clojure.pprint/with-pprint-dispatch new-dispatch + (clojure.pprint/pprint {:role :value :x "abcdef" :hidden? false :distance 0}) + ) + + (clojure.pprint/with-pprint-dispatch new-dispatch + (clojure.pprint/pprint {:role :value :x "abcdef" :hidden? true :distance 0}) + ) + + (clojure.pprint/with-pprint-dispatch new-dispatch + (clojure.pprint/pprint {:role :value :x "abcdef" :hidden? true :distance 1}) + ) + + (let [s (with-out-str + (clojure.pprint/with-pprint-dispatch new-dispatch + (clojure.pprint/pprint + {:role :collection + :x [ + {:role :value :x "a" :hidden? true :distance 1} + {:role :value :x "b" :hidden? false :distance 0} + {:role :value :x "c" :hidden? true :distance 1} + ] + :hidden? false + :distance? 1 + } + ) + )) + ;; This isn't a great idea because this string takes width and therefore will + ;; mess with printing width + + re #"(.*):expound.problems/relevant(.*):expound.problems/relevant(.*)" + ;;[before after] (clojure.string/split s re) + [line before val after] (re-find re s) + ] + + {:s s + :val val + :before before + :after after + } + + (println (str before + val + after + "\n" + " ^^^" + )) + ) + + + + + ;; we could do highlighting not only underneath, but next to value if it is multi-line e.g. > + ;; > + ;; > + ;; Of course, this could be super large for very large values + ;; + ;; making distance and depth different values would of course make it tricky to vary them independently in settings + + + ;; Yes, clojure records can have metadata + (defrecord Foo [x y]) + + (let [f (with-meta (Foo. 1 2) {:bye true}) + + ] + [(.x f) (.y f) (meta f)] + + ) + + ;; HERE - how to embed entire thing in deeper data structure? + + + ) diff --git a/src/expound/printer.cljc b/src/expound/printer.cljc index 137039b3..bc65285a 100644 --- a/src/expound/printer.cljc +++ b/src/expound/printer.cljc @@ -5,6 +5,7 @@ [clojure.set :as set] [expound.util :as util] [expound.ansi :as ansi] + [clojure.pprint :as pprint] #?(:clj [clojure.main :as main])) (:refer-clojure :exclude [format])) @@ -323,3 +324,199 @@ (map #(str (apply str (repeat rest-lines-indent " ")) %)) (into [(str (apply str (repeat first-line-indent " ")) line)]) (string/join "\n"))))) + + +(def marker :expound.problems/relevant) + +(comment + (vector? :xyz) + (def new-dispatch nil)) + +(defmulti new-dispatch + "TODO: name" + (fn [x] + + (if (vector? x) + (first x) + :default + ) + ) + ) + +(defmethod new-dispatch :default [x] + (pprint/simple-dispatch x)) + +;; TODO: rename +(defmethod new-dispatch :scalar [[_ m value]] + + (let [{:keys [distance _depth]} m] + (cond + (zero? distance) + (pprint/pprint-logical-block + (pprint/write-out marker) + (pprint/write-out value) + (pprint/pprint-newline :linear) + (pprint/write-out marker) + (.write ^java.io.Writer *out* " ") + ) + + (:hidden? m) + (pprint/pprint-logical-block + (.write ^java.io.Writer *out* "...") + (.write ^java.io.Writer *out* " ") + (pprint/pprint-newline :linear) + ) + + :else + (pprint/pprint-logical-block + (pprint/write-out value) + (.write ^java.io.Writer *out* " ") + (pprint/pprint-newline :linear) + ) + )) + ) + +(defmethod new-dispatch :vector [[_ m & children]] + (.write ^java.io.Writer *out* "[") + (doseq [x children] + (pprint/write-out x) + ) + (.write ^java.io.Writer *out* "]") + ) + +(defmethod new-dispatch :index [[_ _m _children]] + ;; do not print indexes for now + ) + +(defmethod new-dispatch :value [[_ m v]] + (pprint/write-out [:scalar m v]) + ) + +(defn swidth [s] + (apply max + (map + count + (string/split-lines s))) + ) + +(comment + (let [s "adfasdf\n234\n34"] + ) + + (apply str (repeat 5 " ")) + ) + +;; TODO: rename +(defn hiccup [s] + (map + (fn [x] [:span (string/trim x)]) + (map + string/trimr + (string/split-lines s))) + ) + +(defn highlight-value [s] + (let [re1 #"(.*):expound.problems/relevant(.*):expound.problems/relevant(.*)" + + [line before val after] (re-find re1 s) + [lines_before lines_after] (string/split s (re-pattern (string/re-quote-replacement line))) + indent (apply str (repeat (swidth before) " ")) + underline (apply str (repeat (swidth val) "^")) + ] + (str lines_before + before + val + after + "\n" + indent + underline + lines_after + )) + ) + +(defn value2* [xs] + (pprint/with-pprint-dispatch new-dispatch + (let [out (java.io.StringWriter.)] + (pprint/pprint xs out) + (.toString out))) + ) + +(defn value2 [xs] + (let [s (value2* xs)] + + + (hiccup (highlight-value s)) + + ) + ) + +;; what is the problem(s)? +;; some values don't omit enough information +;; other values omit too much information +;; i can't allow users to vary this while I determine sensible defaults + +;; TODO: do I need to return a list of indented elements? +;; post processing: +;; [:span "fobbar"] + +(comment + (pprint/with-pprint-dispatch new-dispatch + (let [out (java.io.StringWriter.)] + (pprint/pprint {:role :scalar + :value 1 + :distance 0 + :depth 0 + }) + (.toString out)) + ) + + (highlight-value ":expound.problems/relevant1:expound.problems/relevant \n") + + #_(value2 {:role :scalar + :value 1 + :distance 0 + :depth 0 + }) + + (value2 [:scalar + {:distance 0 + :depth 0 + } + 1 + ]) + + (value2* [:vector + {:distance 1} + [:index {:distance 1} 0] + [:value {:distance 1} "a"] + [:index {:distance 1} 1] + [:value {:distance 0} 1] + [:index {:distance 0} 2] + [:value {:distance 0} "b"] + ]) + + (highlight-value "[\"a\"\n :expound.problems/relevant1\n :expound.problems/relevant :expound.problems/relevant\"b\"\n :expound.problems/relevant ]\n") + + (value2 [:vector + {:distance 1} + [:index {:distance 1} 0] + [:value {:distance 1} "a"] + [:index {:distance 1} 1] + [:value {:distance 0} 1] + [:index {:distance 1} 2] + [:value {:distance 1} "b"] + ]) + + (value2 [:vector + {:distance 1} + [:index {:distance 1} 0] + [:value {:distance 1} "a"] + [:index {:distance 1} 1] + [:value {:distance 0} 1] + [:index {:distance 1} 2] + [:value {:distance 1} "b"] + ]) + + + + ) diff --git a/test/expound/printer_test.cljc b/test/expound/printer_test.cljc index 513159ed..a1521413 100644 --- a/test/expound/printer_test.cljc +++ b/test/expound/printer_test.cljc @@ -232,3 +232,157 @@ srows (rest (string/split table #"\n"))]] (is (< (count (last srows)) 200))))) + +(deftest value2 + ;; prints basic value with no highlighting + #_(is (= + [[:span "1"] + [:span "^"]] + (printer/value2 {:role :scalar + :value 1 + :distance 0 + :depth 0 + }) + )) + + (is (= + [[:span "1"] + ] + (printer/value2 + #_{:role :scalar + :value 1 + :distance 0 + :depth 0 + } + [:scalar + {:distance 0 + :depth 0 + } + 1 + ] + ) + )) + ;; should the input be in hiccup-style values + + ["a" 1 "b"] + ;; where 1 is the bad value + + {:role :collection + :distance 1 + :value [ + {:role :value + :value "a" + :distance 1 + :depth 0 + } + {:role :value + :value 1 + :distance 0 + :depth 0 + } + {:role :value + :value "b" + :distance 1 + :depth 0 + } + ] + } + ;; pros: + ;; - collection type is explicit + ;; - no indirection for information about value + ;; cons: + ;; - not sure what happens for custom types e.g. records + ;; - hard to see actual structure + ;; - verbose + + ;; could I flatten the data structure based on well-defined order? + + [{:role :vector + :distance 1 + } + {:role :value + :value "a" + :distance 1 + :depth 0 + } + {:role :value + :value 1 + :distance 0 + :depth 0 + } + {:role :value + :value "b" + :distance 1 + :depth 0 + }] + + ;; no, this doesn't work because we don't know when to "pop" + ;; from the inner vector + + + [:vector + {:distance 1} + [:index {:distance 1} 0] + [:value {:distance 1} "a"] + [:index {:distance 1} 1] + [:value {:distance 0} 1] + [:index {:distance 0} 2] + [:value {:distance 0} "b"] + ] + + ;; pros + ;; - easier to read?? + ;; - might match total format more uniformly (is that important?) + ;; - structure is more directly accessible, not buried inside 'value' key + ;; cons + ;; - 'role' and 'value' are implicit + ;; + + ;; maybe there are levels of stratified design? + ;; 1. one level is semantics of errors + ;; (2)?. another embedded layer is semantics of error values + ;; 3. lower layer is text (similar to fipp) + ;; (there is a layer to convert former to lower) + + + + + ;; what if I just let people register their own + ;; pprint callback for records? still wouldn't help with + ;; array printing or all the rest. + + ;; if i let someone print out their own records, what if + ;; they have an array of records and only one is bad? they + ;; still need to know about distance and depth + + ;; highlights value within collection + + ;; hm, the difference between scalars (the type of data) + ;; and value (the role in the tree) isn't super clear + (is (= + [ + [:span "[[\"a\" 1 \"b\" ]"] + ;;; hmm, the problem here is that the highlighting + ;; is not a normal indentation, it must track the next + ;; value above in a non-normal indentation way, so + ;; it's not clear we can return as data. + ;; we could of course just return a bunch of vectors with whitespace + ;; ahead of them but it's not clear what that gives us + [:span "^"] + ] + (printer/value2 + [:vector + {:distance 1} + [:index {:distance 1} 0] + [:value {:distance 1} "a"] + [:index {:distance 1} 1] + [:value {:distance 0} 1] + [:index {:distance 0} 2] + [:value {:distance 0} "b"] + ] + ) + )) + + + + )