Fix Issue #37 #38
Fix Issue #37 #38
Conversation
| @@ -43,6 +46,14 @@ | |||
| } | |||
| } catch (Exception x) { | |||
| throw new IllegalArgumentException("invalid package"); | |||
| } finally { | |||
| if(jarInputStream != null){ | |||
There was a problem hiding this comment.
Can you fix identation here, please?
There was a problem hiding this comment.
Sorry! I shouldn't have sent this commit. Can I keep it?
|
I'm not sure about this apporach of creating a function that returns null when someone creates a property with a null function. I believe it's a much simpler solution to just change the getValue method. It should check whether the function attribute is null or not instead of checking the value attribute. |
|
I don't know if pass null to #add method is correct. Since #add receives a function, isn't more logical to pass a NullFunction? Sent from my iPhone On 18/10/2013, at 17:32, Arthur Hirata notifications@github.com wrote:
|
|
Actually, "add()" can be called with either a function or a fixed value as argument. Having a "null()" method on the Rule class will not prevent a Property from having an inconsistent state. The reasoning for this is that the current implementation of "getValue()" says that if we don't have a fixed value, we must have a function. However the constructors don't enforce this, even with this pull request. The Property has two contructors: one receives an Object that will be a fixed value to be returned by "getValue()" and the other receives a function that will generate a value to be returned by "getValue()". When we instantiate it with a null, because of the overloading priority, we're actually saying that the function is null and the value is whatever the default is (which happens to be null). So what happens is that we're leaving both the function and the value null and the implementation of "getValue" does not check it accordingly. With this pull request, we're saying that if someone instantiates with a null, it will mean that they want to receive back a null from "getValue()". So we instantiate a Function that returns null. But the same thing wouldn't happen if we called the constructor casting a null to Object as it will still leave us with a null function. What I mean by all this is that the state of a Property is not very well defined nor properly handled and that's what we need to address. Anyways, this pull request will be merged as it fixes leocomelli's use case and as soon as I'm able to I'll try to make things more consistent. Maybe after that, having a shortcut method might be okay. @leocomelli, thanks for the pullrequest and for pointing out this problem! |
|
👍 |
This pull request fixes #37.