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

Add enumerable list of all error objects #59

Merged

Conversation

davewasmer
Copy link
Contributor

Currently, validations.messages exposes a flattened array of all error messages present. It would be useful (for a variety of reasons) to have access to both the message as well as the attribute name it corresponds to.

This PR adds validations.errors following the same pattern as validations.messages (it also adds validations.error).

(docs included)

@rwjblue
Copy link
Member

rwjblue commented Oct 21, 2015

I think this will need some tests. Also, can you tweak the docs to indicate what the contents of .errors will be?

@davewasmer
Copy link
Contributor Author

Good idea - caught a mistake (although I have a sneaking suspicion you knew that already 😉 )

Let me know if you'd like me to squash, or if I missed something in my test coverage.

@offirgolan
Copy link
Collaborator

I like this idea but I don't believe this is the best approach. If you look at how the addon is structured, all validation objects share the same model. Meaning that the global props, validation-result, and validation-result-collection all have (messages, invalid, valid, etc.). All this is currently doing is putting errors on the global validation object.I would suggest to start from validation-result.js and work up.

@davewasmer
Copy link
Contributor Author

@offirgolan glad to hear you like the idea.

So adding an error/errors property to each level? I understand the structure you are describing, but I'm not sure how that would work, since the error objects basically are the validation-result objects.

I could introduce a new concept (an "Error object" for lack of a better name) that is essentially just the message & attribute name, and attach those at each level. Is that what you had in mind?

@offirgolan
Copy link
Collaborator

Since all validations are composed of validation result-collection objects, I feel like we can put the logic there (i dont think you need to touch the result file). So errors can listen to messages.[] and generate a set of error objects

{ 
  attribute: get(this, 'attribute'), 
  message: message
}

Or like you said, we can introduce a new concept addon/validations/error.js

export default Ember.Object.extend({
 attribute: null,
 message: null
})

And just create these. I kind of prefer the latter since it can scale better?

From the global props perspective, it can be identical to how the global messages property is computed.

@rwjblue thoughts?

@offirgolan
Copy link
Collaborator

Actually now that I think of it, the initial error object creation should happen on the result object, not on the result-collection object. So the error prop will listen to message, attribute and isInvalid which will then create an error object with it's own attribute. On the result-collection object, it would just be the same logic as the messages and same for the global prop.

@davewasmer
Copy link
Contributor Author

@offirgolan alrighty, updated. Let me know how that looks.


attribute: null,
message: null

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove these empty lines (before and after the props)

@offirgolan
Copy link
Collaborator

This looks great! If you can just fix that one comment and rebase this to a single commit, I will merge this 😺

}
}),

errors: computed('error', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isnt needed here. A result will only have a single error unless the _validations object get replaced by a validation-result object (only in belongs-to and has-many). Thats why the messages CP exists here but no actually messages prop.

@davewasmer
Copy link
Contributor Author

@offirgolan address both comments and rebased!

@offirgolan
Copy link
Collaborator

Beautiful! 💯 thank you sir!

offirgolan added a commit that referenced this pull request Oct 23, 2015
Add enumerable list of all error objects
@offirgolan offirgolan merged commit cc315ef into adopted-ember-addons:master Oct 23, 2015
@davewasmer
Copy link
Contributor Author

🍻

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

Successfully merging this pull request may close these issues.

None yet

3 participants