six2six / fixture-factory

Generator fake objects from a template
Apache License 2.0
445 stars 88 forks source link

Fix Issue #37 #38

Closed leocomelli closed 10 years ago

leocomelli commented 10 years ago

This pull request fixes #37.

nykolaslima commented 10 years ago

Let's just wait a feedback from one of the guys to choose one syntax. add("name", null); or add("name", null());

@ahirata @aparra

ahirata commented 10 years ago

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.

nykolaslima commented 10 years ago

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:

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.

— Reply to this email directly or view it on GitHub.

ahirata commented 10 years ago

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!

nykolaslima commented 10 years ago

:+1: