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.

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

Right-hand side must be single expression #118

Closed
jonsmock opened this issue Aug 14, 2015 · 8 comments
Closed

Right-hand side must be single expression #118

jonsmock opened this issue Aug 14, 2015 · 8 comments
Milestone

Comments

@jonsmock
Copy link
Contributor

I noticed this rule does not insert the B fact:

(defrule calculate-b
  [A]
  =>
  (println "inserting b")
  (clara/insert! (->B)))

However, this one does insert it:

(defrule calculate-b
  [A]
  =>
  (do (println "inserting b")
      (clara/insert! (->B))))

This could very well be intentional; I don't know what the implications are of allowing multiple s-expressions on the right-hand side. From a new-user perspective (as in, I am a brand new user, as of yesterday), I didn't expect this behavior. I wonder if either an exception could be thrown or if multiple s-expressions could be run.

I see there's already a warning about not including a right-hand side here. This seems like the place to throw for multiple rhs expressions, except that split-lhs-rhs only captures the first expression after the separator, due to destructuring. Obviously, if you want to run all the s-expressions, you'd just have to destructure with [lhs [sep & rhs]] instead.

Anyway, just figured I'd give some feedback. Really liking things so far!

@mrrodriguez
Copy link
Collaborator

I always thought it'd be more intuitive if Clara would just wrap the RHS in a "do" implicitly.

I'm fairly sure there is no good reason to force the rule writer to have to wrap all their s expressions in their own "do" block.

The error is sneaky too. I've been bitten by it before. I'm glad you reported it because I forgot about it. :)

@rbrush
Copy link
Contributor

rbrush commented Aug 14, 2015

I'm fine with implicitly wrapping the RHS in a do. I personally like the explicitly do block, but that's just a style preference and producing unexpected behavior like it does today is definitely a bug.

I'm going to be traveling over the weekend but can get it in early next week. It would be relatively straightforward to add if someone is interest in taking this on as well.

@mrrodriguez
Copy link
Collaborator

I guess I was coming from the Drools background where it is just a sequence of statements on the RHS. If there was an explicit parse error thrown for having more than one form on the RHS, I'd still find that reasonable though.

@rbrush
Copy link
Contributor

rbrush commented Aug 15, 2015

Let's just add an implicit do around the RHS. It seems like the least
surprising behavior and I don't see a good reason not to.

On Friday, August 14, 2015, Mike Rodriguez notifications@github.com wrote:

I guess I was coming from the Drools background where it is just a
sequence of statements on the RHS. If there was an explicit parse error
thrown for having more than one form on the RHS, I'd still find that
reasonable though.


Reply to this email directly or view it on GitHub
#118 (comment).

@jonsmock
Copy link
Contributor Author

Thanks for the responses! I can take a shot at this, unless one of you would rather do it. I can probably get to it later this week?

Is this where you'd want me to add a test for this?

@rbrush
Copy link
Contributor

rbrush commented Aug 17, 2015

Sounds great, Jonathan! I'd love to pull in a contribution along these lines.

And yes, adding a test to clara.test-rules is the right place. (At some point we might break that test namespace up but that's beyond the scope of this change.)

The simplest change is probably just to have the defrule macro add a (do ....) block to wrap the right-hand side if the right-hand side has multiple elements, and update the split-lhs-rhs as you mentioned. We could take update the engine entirely to pass a sequence of expressions from the right-hand side through the compilation process, but this would be more invasive and make management of file and line metadata more complicated during the compilation process.

Thanks!

-Ryan

@mrrodriguez
Copy link
Collaborator

+1 to Jonathan taking a shot at it. I didn't plan to get a chance to work on it right now either.

@rbrush
Copy link
Contributor

rbrush commented Sep 27, 2015

Closing since this was merged and marking for the 0.8.9 release

@rbrush rbrush closed this as completed Sep 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants