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

added explicit forall notation to make TypeApplications easier to use #1006

Merged
merged 8 commits into from
Mar 31, 2020

Conversation

Vlix
Copy link
Contributor

@Vlix Vlix commented Jan 5, 2020

This is a PR for issue #869


Before submitting your PR, check that you've:

  • Bumped the version number

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@Vlix
Copy link
Contributor Author

Vlix commented Jan 5, 2020

The CI is failing because of something in MongoDB?

@parsonsmatt
Copy link
Collaborator

That's a really strange failure...

@Vlix
Copy link
Contributor Author

Vlix commented Jan 5, 2020

All builds using stack work with no problem, but built with cabal, it breaks on a missing MonadFail constraint.

@parsonsmatt
Copy link
Collaborator

I suspect we're missing an upper bound somewhere...

@parsonsmatt
Copy link
Collaborator

Try merging master - there's an upper bound in place to prevent the cabal solver from picking that version.

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.

This is great, thanks! I'm going to see if there are some other breaking changes we can roll into this release.

persistent/ChangeLog.md Outdated Show resolved Hide resolved
persistent/persistent.cabal Outdated Show resolved Hide resolved
@parsonsmatt parsonsmatt added this to the 2.11 milestone Jan 10, 2020
@Vlix
Copy link
Contributor Author

Vlix commented Jan 10, 2020

I'm trying to add some tests, but it's all really annoying.
Also, all of the methods from typeclasses have the backend type variable as number one, because that's the typeclass parameter. Don't really know if there'd be a way to make that more user friendly...

@Vlix
Copy link
Contributor Author

Vlix commented Jan 10, 2020

I guess one way to do it, would be to not export the class methods in Database.Persist, and make new functions that have the same name so anyone importing Database.Persist will be able to use TypeApplications and anyone making their own (e.g. PersistStoreWrite) instance can use Database.Persist.Class

Then again, there are only a few functions that anyone might actually use with TypeApplications, right? The select* ones, and the delete* ones.

@parsonsmatt
Copy link
Collaborator

I wouldn't worry about tests for this. It's not like it can be broken.

You'd only really want to specify the rec where it could be ambiguous, like selectList and deleteWhere. I wouldn't worry too much about duplicating the names. eg insert can't be used polymorphically so it wouldn't make sense to specify the type.

@Vlix
Copy link
Contributor Author

Vlix commented Jan 11, 2020

Well, I did anyway.

But yeah, selectList will have to still be used as selectList @_ @Table, since the backend parameter is first, because of the fact that it's a typeclass method. Same with deleteWhere @_ @Table. :/

@parsonsmatt
Copy link
Collaborator

I think selectList is fine since it's not a class method. selectFirst, count, selectSource are all affected though :\ PersistQueryWrite is also affected, annoying.

Well.

Hmm! Maybe it is worthwhile to have the class methods separate from the functions in the API. It's a breaking change, but all of this is a breaking change anyway.

@Vlix
Copy link
Contributor Author

Vlix commented Jan 11, 2020

Ah true, selectList was outside of the typeclass.

Do you want me to add the select* and delete* functions in Database.Persist?

@parsonsmatt parsonsmatt linked an issue Feb 13, 2020 that may be closed by this pull request
@parsonsmatt
Copy link
Collaborator

looking forward to merging this <3

@parsonsmatt parsonsmatt merged commit 4450d3f into yesodweb:master Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeApplications friendly types
2 participants