Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include line info for steps that raise exceptions #77

Merged
merged 13 commits into from
Jul 29, 2021
Merged

Include line info for steps that raise exceptions #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
553f2a7
log line meta before each substep
philomates Feb 3, 2020
4fff4db
changelog
philomates Feb 3, 2020
5560f02
bump matcher-combinators
philomates Feb 3, 2020
cd6065c
fix test
philomates Feb 3, 2020
83f5648
Merge branch 'master' into pm/line-number-for-exceptions
philomates Feb 4, 2020
096b1b4
fix return value of flow
philomates Feb 4, 2020
ff5b664
fix line number tests
philomates Feb 4, 2020
0817daf
add missing meta call
philomates Feb 4, 2020
4ad2f6f
Merge branch 'master' into pm/line-number-for-exceptions
philomates Jun 17, 2021
0f2fe13
remove consecutive helper
philomates Jun 17, 2021
640e2ee
only print description meta for expr/vars end
philomates Jun 22, 2021
3be8f2e
fix spelling
philomates Jul 19, 2021
050a29d
add exception test
philomates Jul 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changelog

## [5.13.2]
- Include line information when reporting flows that raise exceptions

## [5.13.1]
* unbump major version bump of `timbre` done in `5.13.0`

Expand Down
88 changes: 69 additions & 19 deletions src/state_flow/core.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
(:require [cats.core :as m]
[cats.monad.exception :as e]
[clojure.pprint :as pp]
[clojure.string :as str]
[clojure.string :as string]
[state-flow.internals.description :as description]
[state-flow.state :as state]
[taoensso.timbre :as log]))

Expand Down Expand Up @@ -31,15 +32,16 @@
"Returns a flow that will modify the state metadata.

For internal use. Subject to change."
[description {:keys [line ns file]}]
(modify-meta
(fn [m] (-> m
(update :top-level-description #(or % description))
(update :description-stack (fnil conj [])
(cond-> {:description description
:ns ns}
line (assoc :line line)
file (assoc :file file)))))))
[description {:keys [line ns file call-site-meta]}]
(let [meta-map (cond-> {:description description
:ns ns}
call-site-meta (assoc :call-site-meta call-site-meta)
line (assoc :line line)
file (assoc :file file))]
(modify-meta
(fn [m] (-> m
(update :top-level-description #(or % description))
(update :description-stack (fnil conj []) meta-map))))))

(defn pop-meta
"Returns a flow that will modify the state metadata.
Expand All @@ -54,20 +56,41 @@

(defn description->file
[{:keys [file]}]
(when file (last (str/split file #"/"))))
(when file (last (string/split file #"/"))))

(defn ^:private format-single-description
(defn- format-single-description
[{:keys [line description file] :as m}]
(let [filename (description->file m)]
(str description
(when line
(format " (%s:%s)" filename line)))))
(if filename
(format " (%s:%s)" filename line)
;; TODO: we can probably pull filename info from previous stack entries
(format " (line %s)" line))))))

(defn- remove-non-terminal-call-site-meta
"non-terminal call-site meta is usually redudant with the meta-data provided
by `flow` forms.

It is thus mostly useful at the end of the call-stack, as a way to get more
precise line information for issues that arise after the last `flow` form."
[stack]
(let [call-site-meta? #(contains? % :call-site-meta)
last-call-site-metas (->> stack
reverse
(take-while call-site-meta?)
reverse)
filtered-stack (-> (remove call-site-meta? stack)
(concat last-call-site-metas))]
;; `into` to preserve the `stack` sequence type
(into (empty stack) filtered-stack)))

(defn format-description
[stack]
(->> stack
remove-non-terminal-call-site-meta
(map format-single-description)
(str/join " -> ")))
(string/join " -> ")))

(defn description-stack
"Returns the list of descriptions in the current stack.
Expand All @@ -81,13 +104,13 @@
[s]
(-> s meta :description-stack))

(defn ^:private string-expr? [x]
(defn- string-expr? [x]
(or (string? x)
(and (sequential? x)
(or (= (first x) 'str)
(= (first x) 'clojure.core/str)))))

(defn ^:private state->current-description [s]
(defn- state->current-description [s]
(-> (description-stack s)
format-description))

Expand Down Expand Up @@ -121,6 +144,28 @@
[hook (state/gets (comp :before-flow-hook meta))]
(state/modify (or hook identity))))

(defn- push-abbr-meta [flow]
`(push-meta ~(description/abbr-sexpr flow)
~(assoc (meta flow)
:call-site-meta true)))

(defn- flow-expr? [expr]
(and (coll? expr)
(or (= 'flow (first expr))
(= `flow (first expr)))))

(defn annotate-with-line-meta [flows]
(let [annotated-flows (reduce (fn [acc flow]
(if (flow-expr? flow)
(conj acc flow) ;; `flow`s push their own meta data
(into [] (concat acc [(push-abbr-meta flow) flow `(pop-meta)]))))
[]
flows)]
;; to preserve the return value, exclude terminal pop-meta's
(if (= `(pop-meta) (last annotated-flows))
(butlast annotated-flows)
annotated-flows)))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Public API

Expand All @@ -137,12 +182,17 @@
(throw (IllegalArgumentException. "The first argument to flow must be a description string")))
(when (vector? (last flows))
(throw (ex-info "The last argument to flow must be a flow/step, not a binding vector." {})))
(let [flow-meta caller-meta
flows' (or flows `[(m/return nil)])]
(let [flow-meta caller-meta
annotated-flows (annotate-with-line-meta
(or flows `[(m/return nil)]))
pop-line-meta (if (flow-expr? (last annotated-flows))
'()
`((pop-meta)))]
`(m/do-let
(push-meta ~description ~flow-meta)
(apply-before-flow-hook)
[ret# (m/do-let ~@flows')]
[ret# (m/do-let ~@annotated-flows)]
~@pop-line-meta
(pop-meta)
(m/return ret#))))

Expand Down
22 changes: 22 additions & 0 deletions src/state_flow/internals/description.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
(ns state-flow.internals.description
(:require [clojure.string :as string]))

(def ^:private abbr-size 15)
(defn- abbr-list [expr-str ellipse-end]
(let [[head & tail] (string/split expr-str #" ")]
(if (empty? tail)
expr-str
(str head ellipse-end))))

(defn- ellipsify [expr-str]
(case (first expr-str)
\( (abbr-list expr-str " ...)")
\[ (abbr-list expr-str " ...]")
expr-str))

(defn abbr-sexpr [expr]
(let [expr-str (str expr)]
(if (< abbr-size (count expr-str))
(ellipsify expr-str)
expr-str)))

2 changes: 1 addition & 1 deletion test/state_flow/assertions/matcher_combinators_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
:match/actual {:n 2}}
flow-ret)))
(testing "saves assertion report to state with current description stack"
(is (match? {:flow/description-stack [{:description "match?"}]
(is (match? {:flow/description-stack [{:description "match?"} {:description "(cats.core/do-let ...)"}]
:match/result :mismatch
:mismatch/detail {:n {:expected 1 :actual 2}}
:probe/results [{:check-result false :value {:n 2}}
Expand Down
50 changes: 24 additions & 26 deletions test/state_flow/core_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,22 +159,13 @@
(let [add-two-fn (state-flow/as-step-fn (state/modify #(+ 2 %)))]
(is (= 3 (add-two-fn 1)))))

(defn consecutive?
"Returns true iff ns (minimum of 2) all increase by 1"
[& ns]
(and (>= (count ns) 2)
(let [a (first ns)
z (inc (last ns))]
(and (< a z)
(= (range a z) ns)))))

(deftest current-description
(testing "top level flow"
(is (re-matches #"level 1 \(core_test.clj:\d+\)"
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> \(state-flow\/current-description\) \(line \d+\)"
(first (state-flow/run (flow "level 1" (state-flow/current-description)))))))

(testing "nested flows"
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> level 2 \(core_test.clj:\d+\)"
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> level 2 \(core_test.clj:\d+\) -> \(state-flow\/current-description\) \(line \d+\)"
(first (state-flow/run (flow "level 1"
(flow "level 2"
(state-flow/current-description)))))))
Expand All @@ -187,27 +178,28 @@
(flow "level 2"
(flow "level 3"
(state-flow/current-description)))))]
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> level 2 \(core_test.clj:\d+\) -> level 3 \(core_test.clj:\d+\)" desc))
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> level 2 \(core_test.clj:\d+\) -> level 3 \(core_test.clj:\d+\) -> \(state-flow\/current-description\) \(line \d+\)"
desc))
(testing "line numbers are correct"
(let [[level-1-line
level-2-line
level-3-line]
(->> desc
(re-find #"level 1 \(core_test.clj:(\d+)\) -> level 2 \(core_test.clj:(\d+)\) -> level 3 \(core_test.clj:(\d+)\)")
(re-find #"level 1 \(core_test.clj:(\d+)\) -> level 2 \(core_test.clj:(\d+)\) -> level 3 \(core_test.clj:(\d+)\) -> \(state-flow\/current-description\) \(line \d+\)")
(drop 1)
(map #(Integer/parseInt %)))]
(is (consecutive? line-number-before-flow-invocation
level-1-line
level-2-line
level-3-line))))))
(is (<= line-number-before-flow-invocation
level-1-line
level-2-line
level-3-line))))))

(testing "composition"
(let [line-number-before-flow-invocation (this-line-number)
level-3 (flow "level 3" (state-flow/current-description))
level-2 (flow "level 2" level-3)
level-1 (flow "level 1" level-2)
[desc _] (state-flow/run level-1)]
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> level 2 \(core_test.clj:\d+\) -> level 3 \(core_test.clj:\d+\)"
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> level 2 \(core_test.clj:\d+\) -> level 3 \(core_test.clj:\d+\) -> \(state-flow\/current-description\) \(line \d+\)"
desc))
(testing "line numbers are correct, even when composed"
(let [[level-1-line
Expand All @@ -217,27 +209,33 @@
(re-find #"level 1 \(core_test.clj:(\d+)\) -> level 2 \(core_test.clj:(\d+)\) -> level 3 \(core_test.clj:(\d+)\)")
(drop 1)
(map #(Integer/parseInt %)))]
(is (consecutive? line-number-before-flow-invocation
level-3-line
level-2-line
level-1-line))))))
(is (<= line-number-before-flow-invocation
level-3-line
level-2-line
level-1-line))))))

(testing "after nested flows complete"
(testing "within nested flows "
(is (re-matches #"level 1 \(core_test.clj:\d+\)"
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> \(state-flow\/current-description\) \(line \d+\)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I run this I get

=> "level 1 (core_test.clj:2) -> (state-flow/current-description) (line 4)"

Where line 4 is coming from?

(first (state-flow/run (flow "level 1"
(flow "level 2")
(state-flow/current-description))))))
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> level 2 \(core_test.clj:\d+\)"
(is (re-matches #"level 1 \(core_test.clj:\d+\) -> level 2 \(core_test.clj:\d+\) -> \(state-flow\/current-description\) \(line \d+\)"
(first (state-flow/run (flow "level 1"
(flow "level 2"
(flow "level 3")
(state-flow/current-description)))))))
(is (re-matches #"level 1 \(core_test.clj:\d+\)"

(is (re-matches #"level 1 \(core_test.clj:\d+\) -> \(state-flow\/current-description\) \(line \d+\)"
(first (state-flow/run (flow "level 1"
(flow "level 2"
(flow "level 3"))
(state-flow/current-description)))))))))
(state-flow/current-description))))))))
(testing "description in presence of exceptions"
(is (re-matches #"will boom \(core_test.clj:\d+\) -> root \(core_test.clj:\d+\) -> child2 \(core_test.clj:\d+\) -> bogus"
(-> (state-flow/run (flow "will boom" bogus-flow) {:value 2})
second
(#'state-flow/state->current-description))))))

(deftest top-level-description
(let [tld (fn [flow] (->> (state-flow/run flow)
Expand Down