Darryl L. Pierce commented on PROTON-743:

Thank you for the patch! Here's some feedback from reading it:

Is there any need for the impl to be available to anything outside of the 
object? I would suggest removing the attr_reader :impl until such a time as 
there's a need to expose that detail outside of the class.

I think the methods to set errors on an object, such as Messenger, should 
ultimately be in Messenger itself rather than being done through an instance of 
another class. Unless the Messenger class is going to have a single instance of 
Error to represent the most recent error, I'm not sure there's much benefit to 
having an external object be able to change the state that way.

The Error method shouldn't really interact with the underlying error source. It 
should be stateful and have the error code and error message given to it as a 
part of its initializer, rather than giving it a reference to the object 
itself. Imagine:

1. Messenger experiences a problem raises an error resulting in e[0] being 
2. The code beings to display details of e[0], but doesn't get to the text 
message, but
3. Messenger experiences another problem and raises error e[1], so
4. When the code shows the text via e[0] it shows the current error's message 
which is actually for e[1].

So unless there's only going to be one instance of Qpid::Proton::Error for each 
instance of Qpid::Proton::Messenger, which doesn't seem to be the intent, then 
it's very likely that an Error object will return incorrect information.

> [PATCH] ruby: user doesn't have access to clear messenger error object
> ----------------------------------------------------------------------
>                 Key: PROTON-743
>                 URL: https://issues.apache.org/jira/browse/PROTON-743
>             Project: Qpid Proton
>          Issue Type: Bug
>          Components: ruby-binding
>    Affects Versions: 0.8
>            Reporter: Dominic Evans
>            Assignee: Darryl L. Pierce
>            Priority: Minor
>         Attachments: 
> 0001-Wrapper-pn_error-so-user-has-more-control-over-it.patch
> Once messenger error has been set and it has been read from messenger.error 
> and logged / acted upon, it would be useful for the user to be able to clear 
> the error object so they can continue operating. Wrappering it up in an Error 
> class makes this easier.

This message was sent by Atlassian JIRA

Reply via email to