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

Apply entity builder on setter. #30

Open
guisouza opened this issue Jul 5, 2017 · 2 comments
Open

Apply entity builder on setter. #30

guisouza opened this issue Jul 5, 2017 · 2 comments
Labels

Comments

@guisouza
Copy link
Contributor

guisouza commented Jul 5, 2017

Guys I really hope I am wrong, but i think we are not applying entity builders on setter, but only on entity construction ..

Is this the expected behaviour ? or it is a bug ?

@albertossilva could you please check, i have a fix for it, like this :

    set: function (value){
      if (instance.schema[field].type || instance.schema[field].builder) {
        value = instance.applyEntityConstructor(instance.schema[field], value);
      }
      if(instance.data[field] !== value) {
        instance.data[field] = value;
        return instance._validate();
      }
    },

should I submit the PR ?

@albertossilva
Copy link
Collaborator

@guisouza Yes, we are not applying it is nice improvement... I would also check if value is already instanceOf instance.schema[field].type.
And.... let's avoid the value override at the second line please, ahahaa, 😛

@guisouza guisouza added the bug label Jul 6, 2017
@guisouza
Copy link
Contributor Author

guisouza commented Jul 6, 2017

good point .. First of all, check if its already instantiated ..

Now thinking .. should we pop an error in cases which the constructor of a entity returned by the builder differs from the declared Type ?
Or when we have a custom builder, the Type is just a documentation, IMHO, No! after all we are passing the Type to every builder ..

@albertossilva its getting complex .. #helpwanted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants