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

refactor: convert Validator to Class #90

Conversation

alexlafroscia
Copy link
Collaborator

@alexlafroscia alexlafroscia commented Dec 26, 2018

The "meat" of this change converts the existing Validator implementation into a class. Specifically, Validators are now Monoids that can be combined to create more complex validations:

This refactors the concept of what a Validator actually is, changing it from a Function into a Class. This allows for co-locating all of the information about how to check a value and format the error message into a single location.

The Validator class can be treated as a Monoid, meaning that it can be combined with other instances of itself to build up more complex Validators. Many operations that we supported previously can be thought of as combinations of other Validators, so I thought that formalizing that made sense. With base combinators like and and or, we can build up some of the type modifiers that we support like arrayOf or oneOf. All of these can be expressed as Validators, in terms of other Validators.

Note: This will conflict with some of the changes made in #88. I'll refactor whichever pull request lands later so that they're compatible.

@alexlafroscia alexlafroscia force-pushed the convert-validator-to-monoid branch from ea2357c to a514ae3 Compare December 26, 2018 21:15
@alexlafroscia
Copy link
Collaborator Author

alexlafroscia commented Dec 26, 2018

I wanted to expose the base Validator class to allow users to extend it themselves and create custom validators, but I'm not sure that's possible with the current state of the babel-plugin-filter-imports. Those code trips up the plugin:

import { Validator } from '@ember-decorators/argument';

class SomeValidator extends Validator {}

With good enough dead code elimination, it should be safe to support this; we filter out the use of @argument everywhere, which removes all usage of SomeValidator, which means that Validator is no longer used and can also be pruned. However, this doesn't seem to be supported by babel-plugin-filter-imports at this time and would be easy to implement in the future, should we figure out how to support it.

@alexlafroscia alexlafroscia force-pushed the convert-validator-to-monoid branch from a514ae3 to 7bc4852 Compare December 26, 2018 21:26
@alexlafroscia alexlafroscia changed the title Convert Validator to Class refactor: convert Validator to Class Dec 27, 2018
Copy link
Contributor

@pzuraq pzuraq left a comment

Choose a reason for hiding this comment

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

👍 This looks good, I think the classes really clean things up in general, nice job!

This refactors the concept of what a Validator actually is, changing it from a Function into a Class. This allows for co-locating all of the information about how to check a value and format the error message into a single location.

The Validator class can be treated as a Monoid, meaning that it can be combined with other instances of itself to build up more complex Validators. Many operations that we supported previously can be thought of as combinations of other Validators, so I thought that formalizing that made sense. With base combinators like `and` and `or`, we can build up some of the type modifiers that we support like `arrayOf` or `oneOf`. All of these can be expressed as Validators, in terms of other Validators.
@alexlafroscia alexlafroscia force-pushed the convert-validator-to-monoid branch from 837c5fb to 29ddb02 Compare January 7, 2019 18:48
@alexlafroscia alexlafroscia merged commit 81ae6c1 into ember-decorators:master Jan 7, 2019
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.

2 participants