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.

Sign up for GitHub

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

Jump to bottom

Implement Type Literal based field definitions #1343

Merged
merged 11 commits into from
Apr 12, 2022
Merged

Implement Type Literal based field definitions #1343

merged 11 commits into from
Apr 12, 2022

Conversation

danbroooks
Copy link
Contributor

@danbroooks danbroooks commented Nov 28, 2021 •
edited
Loading

Implements #562

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)

danbroooks
danbroooks commented Nov 28, 2021
View reviewed changes
persistent/Database/Persist/Types.hs Outdated
@@ -60,6 +62,7 @@ import Database.Persist.Types.Base
, FieldCascade(..)
, FieldDef(..)
, FieldType(..)
, FieldTypeLit(..)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the comment at the top here:

persistent/persistent/Database/Persist/Types.hs

Lines 48 to 50 in 77545dc

-- this module is a bit of a kitchen sink of types and concepts. the guts of
-- persistent, just strewn across the table. in 2.13 let's get this cleaned up
-- and a bit more tidy.

I wasn't sure if this was the right place for this import, but seemed like it was where everything else was re-exported (I need to use this type in Persist/TH.hs I think...)

Going forward should I import this from Database.Persist.Types.Base ? Or is re-exporting it here correct for the time being?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather not expose data constructors to the end-user in a non Internal module, since any change to those constructors necessitates a breaking change to the API version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, I'd be fine exporting it from Database.Persist.Quasi.Internal, since that's where it is used.

danbroooks
danbroooks commented Nov 28, 2021
View reviewed changes
persistent/test/Database/Persist/QuasiSpec.hs
[ Token "one"
, Token "Finite 1"
]
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an initial test I did as an exploratory thing to verify the parser was even parsing numbers at all to begin with (which it was), this test could potentially be removed now. Though it may also be worthwhile leaving in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah! It's good to have around as a regression. Thanks for the test!

@danbroooks danbroooks force-pushed the persist-entity-type-lits branch 2 times, most recently from 8cf0393 to 85e2ee8 Compare November 28, 2021 21:18
parsonsmatt
parsonsmatt approved these changes Jan 27, 2022
View reviewed changes
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.

Thanks! This is great 😂

OK, so, since this is modifying a constructor on an exposed type (FieldType), then it's a major version bump. So it'll go in with persistent-2.14.

persistent/Database/Persist/Quasi/Internal.hs
Comment on lines +103 to +107
<|> parseParenEnclosed x xs
<|> parseList x xs
<|> parseNumericLit x xs
<|> parseTextLit x xs
<|> parseTypeCon x xs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice - this is great. Moving away from ad-hoc text munging and instead using more idiomatic Haskell classes.

persistent/Database/Persist/Types.hs Outdated
@@ -60,6 +62,7 @@ import Database.Persist.Types.Base
, FieldCascade(..)
, FieldDef(..)
, FieldType(..)
, FieldTypeLit(..)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather not expose data constructors to the end-user in a non Internal module, since any change to those constructors necessitates a breaking change to the API version.

persistent/Database/Persist/Types.hs Outdated
@@ -60,6 +62,7 @@ import Database.Persist.Types.Base
, FieldCascade(..)
, FieldDef(..)
, FieldType(..)
, FieldTypeLit(..)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, I'd be fine exporting it from Database.Persist.Quasi.Internal, since that's where it is used.

persistent/test/Database/Persist/QuasiSpec.hs
[ Token "one"
, Token "Finite 1"
]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah! It's good to have around as a regression. Thanks for the test!

persistent/test/Database/Persist/TH/TypeLitFieldDefsSpec.hs
|]

spec :: Spec
spec = describe "TypeLitFieldDefs" $ do
Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome specs

@parsonsmatt
Copy link
Collaborator

Oh - this also needs a bump/changelog entry for persistent-test. And it may be a good idea to start putting a lower bound on persistent-test for the test-suites of the other packages.

@parsonsmatt parsonsmatt added this to the 2.14 milestone Jan 27, 2022
@danbroooks danbroooks force-pushed the persist-entity-type-lits branch 3 times, most recently from cc95228 to 929bc97 Compare March 16, 2022 19:02
@parsonsmatt
Copy link
Collaborator

hmm, we have a test failure now. would like to get this merged in before 2.14, will try and get it sorted if you can't get to it soon

thanks!

@parsonsmatt parsonsmatt changed the base branch from master to matt/finalize-type-lit-pr April 12, 2022 04:18
@parsonsmatt parsonsmatt merged commit c3886bf into yesodweb:matt/finalize-type-lit-pr Apr 12, 2022
@parsonsmatt parsonsmatt mentioned this pull request Apr 12, 2022
Merged
7 tasks
parsonsmatt added a commit that referenced this pull request Apr 12, 2022
@parsonsmatt @danbroooks
Finalize type lits (#1388)
c3f0577 
* Implement Type Literal based field definitions  (#1343)

* Implement typelit field definitions

* Some cleanup

* Sort indentation and add PR link in changelog

* Move location of exposed type

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

* fix tests

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
Reviewers

@parsonsmatt parsonsmatt parsonsmatt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.14
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@danbroooks @parsonsmatt