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

Fix for #227 #233

Merged
merged 2 commits into from
May 4, 2017
Merged

Fix for #227 #233

merged 2 commits into from
May 4, 2017

Conversation

bryceosterhaus
Copy link
Member

We need to explicitly handle the use case for each validator since they all work differently. Validators that accept arguments all do something a little different. Previously, any validator that took an argument was never throwing an error.

We need to explicity handle use case for each validator since they all work differently. Validators that accept arguments all do something a little different.
};
}

setPrimitiveValidators('any');
Copy link

Choose a reason for hiding this comment

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

Would it make more sense to make these (and shapeOf, oneOfType, etc.) just be methods on the Config as defined above? As oppose to declaring the object, and then mutating it down here over and over.

The implementation of each method could still just call some helper for the behavior that is shared.

Another idea would be to declare another object, something like ConfigValidators, and just merge the two objects at the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regardless this proposal the end behavior should be the same, correct? If so this pull looks good to me and then if you want you can send a refactor for the internals.

Copy link

Choose a reason for hiding this comment

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

Yes, it would be the same.

It would just make this file much easier to read, since all methods on Config would be explicitly declared in the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Merging this one and refactoring is welcome. Thx.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I will go ahead and refactor and send another PR.

@eduardolundgren eduardolundgren merged commit ce341ef into metal:master May 4, 2017
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.

3 participants