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 readme #12

Merged
merged 1 commit into from
Jul 30, 2018
Merged

Conversation

vyorkin
Copy link
Contributor

@vyorkin vyorkin commented Jul 30, 2018

First, I want to say thank you for this awesome package 👍

What does this pull request do?

I've just tried to follow the "overview" section and noticed a couple of inconsistencies: a couple of copy/paste typos and a few missing types. It may be a bit hard to follow without knowing the exact types of ValidationError, Encrypted etc, so I've added them. Also, I've added validateNonEmpty and validateMinLength functions, so now it should be kinda self-contained and easier to follow.

Other Notes:

Maybe it makes sense to include the code from the Overview in examples? Here is a gist: https://gist.github.com/vyorkin/9dabae99114a5604717f986be36ff31b
Another thing that might be confusing for newcomers like me is the identity-trick, I mean validator = applyOnInputFields (identity { name: V.nonEmpty, ... })

@vyorkin
Copy link
Contributor Author

vyorkin commented Jul 30, 2018

I'll update the PR to include the expandable blocks with full types and validation functions as we've discussed in Slack. Some examples below.


First example:

type Errors = Array ValidationError

newtype Form f = Form
  { name      :: f Errors String String
  , password1 :: f Errors String Encrypted
  , password2 :: f Errors String Encrypted
  }
derive instance newtypeForm :: Newtype (Form f) _
Expand to see the definition of ValidationError and Encrypted types
newtype Encrypted = Encrypted String

data ValidationError
  = Required
  | TooShort Int
  | EncryptionFailed

derive instance genericVaidationError :: Generic ValidationError _
instance showValidationError :: Show ValidationError where show = genericShow

Second example:
import Polyform.Validation (Validation(..), hoistFnV)

-- An example Polyform validator
validateEncrypt :: forall m. Monad m => Validation m Errors String Encrypted
validateEncrypt = hoistFnV \str ->
  if null str
     then Invalid [EncryptionFailed]
     else pure (Encrypted str)

We can provide a field validation function like this to every field that will be in our form, and then use the applyOnInputFields helper function from Formless to convert these functions to run on the input field and store their result in the result field:

import Formless.Validation.Polyform (applyOnInputFields)

-- Each validator in the record should have the type `Validation m error input output`
validator :: forall m. MonadEffect m => Form F.InputField -> m (Form F.InputField)
validator = applyOnInputFields $ identity
  { name: validateNonEmpty
  , password1: validateMinLength 8 *> validateEncrypt
  , password2: validateMinLength 8 *> validateEncrypt
  }
Click to see how validateNonEmpty and validateMinLength are defined
validateNonEmpty :: forall m. Monad m => Validation m Errors String String
validateNonEmpty = hoistFnV \str ->
  if null str
     then Invalid [Required]
     else pure str

validateMinLength :: forall m. Monad m => Int -> Validation m Errors String String
validateMinLength n = hoistFnV \p ->
  let p' = String.length p
  in if p' < n
     then Invalid [TooShort n]
     else pure p

@vyorkin
Copy link
Contributor Author

vyorkin commented Jul 30, 2018

Ready for review (I'll squash the commits at the end)

readme.md Outdated
unwrapOutputs' = F.unwrapOutputs
-- `unwrapOutput` is a helper function that will unwrap all these newtypes on your behalf.
-- Used on our custom Form type, it'd apply this transformation:
unwrapOutput' :: Form F.OutputField -> FormOutput
Copy link
Owner

Choose a reason for hiding this comment

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

FormOutput isn't actually a type that's been defined in this README. I think I'd like to keep it as the record, no type alias, to make it clear that the output is just the fields with their output values at the end, nothing fancier than that. What do you think?

readme.md Outdated
```

</details>

Copy link
Owner

Choose a reason for hiding this comment

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

Mind adding an extra newline here? Looks a bit squashed in the markdown viewer.

Copy link
Owner

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up, @vyorkin! I have a couple comments in here, but otherwise this should be good to merge.

I noticed this is under the @vyorkin-forks account. Do you want to open a different PR under your @vyorkin account so that handle is listed as a contributor to the repo instead? Up to you.

@vyorkin
Copy link
Contributor Author

vyorkin commented Jul 30, 2018

ah, I think its ok, I hope I'll be able to make more valuable contributions in the future :)

* Fix typos
* Add missing types and functions
@vyorkin
Copy link
Contributor Author

vyorkin commented Jul 30, 2018

Squashed commits, I think its ready

@thomashoneyman thomashoneyman merged commit dc87a1e into thomashoneyman:master Jul 30, 2018
@thomashoneyman
Copy link
Owner

Thanks!

@vyorkin vyorkin deleted the readme-fixes branch July 30, 2018 22:19
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