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

Reduce verbosity using Monadic Forms #1432

Merged
merged 1 commit into from Aug 22, 2017

Conversation

Projects
None yet
2 participants
@sestrella
Contributor

sestrella commented Aug 21, 2017

Building forms where some fields validations depend on other fields is simple using Monadic Forms, but as the own Yesod website mentioned:

it does so at the cost of being more verbose.

Some of this verbosity is caused by the need of grouping the FiedView site returned by either mreq or mopt. Also on the other hand, formToAForm requires a Monadic Form like MForm m (FormResult a, [FieldView site]) to able to transform to AForm m a. Consider the following scenario:

formToAForm $ do
  (field1F, field1V) <- mreq textField MsgField1 Nothing
  (field2F, field2V) <- mreq (checkWith field1F textField) MsgField2 Nothing
  (field3F, field3V) <- mreq (checkWith field1F textField) MsgField3 Nothing
  return
    ( MyForm <$> field1F
                     <*> field2F
                     <*> field3F
    , [field1V, field2V, field3V]
    )

While the previous example could be refactored to use Applicative Form style, it requires combining the fields in order to be able to run a custom check. Doing that for more than one field that depends on one value is a little bit tricky using Applicative style.

I'll like to propose adding the following variant of a Monadic Form:

type WForm m a = MForm (WriterT [FieldView (HandlerSite m)] m) a

And the correspondent req and opt functions like:

wreq :: (...) => Field m a -> FieldSettings site -> Maybe a -> WForm m (FormResult a)

wopt :: (...) => Field m a -> FieldSettings site -> Maybe (Maybe a) -> WForm m (FormResult (Maybe a))

wFormToAForm ::  WForm  m (FormResult a) -> AForm m a

The example above then could be rewritten as follows:

wFormToAForm $ do
  field1F <- wreq textField MsgField1 Nothing
  field2F <- wreq (checkWith field1F textField) MsgField2 Nothing
  field3F <- wreq (checkWith field1F textField) MsgField3 Nothing
  return $
      MyForm <$> field1F
                    <*> field2F
                    <*> field3F

I'll be more than happy to submit a PR if this proposal makes sense to the maintainers.

Issue: #1432

@sestrella

This comment has been minimized.

Contributor

sestrella commented Aug 21, 2017

@snoyberg would you mind taking a look, please?

@snoyberg

This looks great, just one minor request for changing regarding Since comment format. Thank you!

-- | Converts a form field into monadic form 'WForm'. This field requires a
-- value and will return 'FormFailure' if left empty.
--
-- Since 1.4.14

This comment has been minimized.

@snoyberg

snoyberg Aug 22, 2017

Member

Minor nitpick, but would you be able to convert this to the new style @since comments?

@@ -1,5 +1,5 @@
name: yesod-form
version: 1.4.13
version: 1.4.14

This comment has been minimized.

@snoyberg

snoyberg Aug 22, 2017

Member

Bonus points for handling the version bump and changelog, much appreciated!

@@ -150,6 +150,18 @@ main = hspec $ do
addToken
statusIs 200
bodyEquals "12345"
yit "labels WForm" $ do

This comment has been minimized.

@snoyberg

snoyberg Aug 22, 2017

Member

Triple bonus points for test cases!

@sestrella sestrella force-pushed the stackbuilders:reduce_verbosity_using_monadic_forms branch from 2a59e5f to 0f28604 Aug 22, 2017

@sestrella

This comment has been minimized.

Contributor

sestrella commented Aug 22, 2017

@snoyberg thank you very much for reviewing this PR. All comments have been addressed.

@snoyberg snoyberg merged commit 469c1c2 into yesodweb:master Aug 22, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@snoyberg

This comment has been minimized.

Member

snoyberg commented Aug 22, 2017

Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment