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

Add constraint to parser for empty :test conditions #349

Merged
merged 2 commits into from
Oct 17, 2017
Merged

Add constraint to parser for empty :test conditions #349

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97e88e1
Add constraint to parser for empty :test conditions
Oct 11, 2017
017d5ab
Address Will's comments
Oct 17, 2017
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
7 changes: 6 additions & 1 deletion src/main/clojure/clara/rules/dsl.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,12 @@
(parse-expression nested-expr expr-meta)))

(contains? #{'test :test} (first expression))
{:constraints (vec (rest expression))}
(if (= 1 (count expression))
(throw-dsl-ex (str "Empty test conditions are not allowed.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this might be clearer with a colon in front of the "test" to denote that it is a keyword, i.e.

"Empty :test conditions are not allowed."

I realize that the symbol "test" is allowed as well, but I suspect that :test is the more common usage since that is what the docs use.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya good point

{}
expr-meta)
{:constraints (vec (rest expression))})


Copy link
Collaborator

Choose a reason for hiding this comment

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

Throw-dsl-ex won't include useful info here like what the offending rule or query is, but that looks like a shortcoming of the current usage of throw-dsl-ex in this namespace that impacts other malformed productions as well, so I think changing that is out of scope here. I'll log another issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference the issue logged is #351

:default
(parse-condition-or-accum expression expr-meta)))
Expand Down
11 changes: 11 additions & 0 deletions src/test/clojure/clara/test_rules.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,6 +1133,17 @@
fire-rules
(query not-different-temps))))))

(deftest test-empty-test-condition

(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"line.*123.*column.*456"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a particular reason why you didn't use assert-ex-data here or were you just not aware of it? It has the advantages of:

  • Using normal Clojure data equality semantics rather than messy string comparisons.
  • Not failing if something else wraps the exception later since it walks the exception chain to look for a match.

Side note: Clojure has an IExceptionInfo interface, of which clojure.lang.ExceptionInfo is a concrete implementation. Any instance checks you do should generally use IExceptionInfo for all the usual reasons to favor coding against an interface rather than the implementation; in this particular case my understanding is that user programs might define implementations of IExceptionInfo themselves. The ex-data function just returns nil if the exception is of the wrong type so that can be used as well in many cases in logic like

(when (ex-data e) ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasnt aware of it. I copied a test elsewhere in this file and modified it for my needs. I'll work it out to use this.

(dsl/parse-query*
[]
[[:test]]
{}
{:line 123 :column 456}))))

(deftest test-multi-insert-retract

(is (= #{{:?loc "MCI"} {:?loc "BOS"}}
Expand Down
9 changes: 4 additions & 5 deletions src/test/common/clara/test_dsl.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,25 +299,25 @@
#?(:clj
(def-rules-test test-rhs-locals-shadowing-vars

{:rules [r1 [[[:test]]
{:rules [r1 [[]
(let [{:keys [locals-shadowing-tester]} {:locals-shadowing-tester :good}]
(insert! ^{:type :result}
{:r :r1
:v locals-shadowing-tester}))]

r2 [[[:test]]
r2 [[]
(let [locals-shadowing-tester :good]
(insert! ^{:type :result}
{:r :r2
:v locals-shadowing-tester}))]

r3 [[[:test]]
r3 [[]
(let [[locals-shadowing-tester] [:good]]
(insert! ^{:type :result}
{:r :r3
:v locals-shadowing-tester}))]

r4 [[[:test]]
r4 [[]
(insert-all! (for [_ (range 1)
:let [locals-shadowing-tester :good]]
^{:type :result}
Expand All @@ -337,7 +337,6 @@
{:r :r4
:v :good}])
(->> (-> empty-session
(insert ^{:type :test} {})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose fixing this by changing the fact type inserted here to something other than :test rather than eliminating the LHS of the rule. My reasoning is that a test for shadowing seems a bit stronger if we force the evaluation to wait until the insert call is made (and not at any time after the call to create the session). Not a big deal, but it is also an easy change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can do

fire-rules
(query q))
(map :?r)
Expand Down