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

Support declaring Maybe before the type in model definitions #1264

Merged

Conversation

danbroooks
Copy link
Contributor

@danbroooks danbroooks commented May 6, 2021

Before submitting your PR, check that you've:

  • Documented new APIs with Haddock markup
  • Added @since declarations to the Haddock
  • Ran stylish-haskell on any changed files.
  • Adhered to the code style (see the .editorconfig file for details)

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Bumped the version number if there isn't an (unreleased) on the Changelog
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

Closes #277

@danbroooks danbroooks changed the title Support declaring Maybe before the type Support declaring Maybe before the type in model definitions May 6, 2021
Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

Let's write a test in persistent-test that'll be run on the various backends and ensure that migrations are getting handled correctly.

I expect that they are, but it's good to double check :)

> nullableField Int nullable

However the difference here is in the first instance the Haskell type will be 'Maybe Int',
but in the second it will be 'Int'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should add a note that this will cause runtime errors if the database returns NULL and the PersistField instance for the type in question doesn't handle PersistNull.

> TableName
> fieldName FieldType
> otherField String
> nullableField (Maybe Int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome

@@ -176,6 +178,7 @@ spec = describe "THSpec" $ do
SharedPrimaryKeySpec.spec
SharedPrimaryKeyImportedSpec.spec
ImplicitIdColSpec.spec
MaybeFieldDefsSpec.spec
Copy link
Collaborator

Choose a reason for hiding this comment

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

okay awesome

This is testing that the TH generation does the right thing for the Haskell side

@parsonsmatt
Copy link
Collaborator

CI is fixed on master

@danbroooks danbroooks force-pushed the quasi-refactor-types branch 2 times, most recently from 36a8a11 to 9fac010 Compare May 13, 2021 17:27
@danbroooks danbroooks marked this pull request as ready for review May 13, 2021 19:40
@parsonsmatt parsonsmatt added this to the 2.13.1.0 milestone Jun 18, 2021
@parsonsmatt
Copy link
Collaborator

Ah, now we're failing because of MongoDB. I'm going to merge this into a branch and finish it. Thanks so much, and sorry it took me so long to review!

@parsonsmatt parsonsmatt changed the base branch from master to matt/maybe-before-type June 29, 2021 13:28
@parsonsmatt parsonsmatt merged commit 395951b into yesodweb:matt/maybe-before-type Jun 29, 2021
parsonsmatt added a commit that referenced this pull request Jun 29, 2021
* Support declaring Maybe before the type in model definitions (#1264)

* Add missing type sigs

* Extract/dry up is nullable

* Extract/rename associateComments function

* Implement a way of detecting Maybe field defs

* Fixing warnings

* Add a spec

* Add documentation

* Add PR reference to changelog

* Add a test to cover the various backends

* Also test mongo

* Fix test compilation issues

* Update readme

Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>

* gotta fix that

Co-authored-by: Dan Brooks <danbroooks@users.noreply.github.com>
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.

support Maybe before the type
2 participants