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

Update mkYesodWith and refactor so that mkYesod uses the context parser #1478

Merged
merged 3 commits into from
Jan 29, 2018

Conversation

jprider63
Copy link
Contributor

Since it looks like you're planning on introducing breaking changes, it seems like a good time for this. This PR is breaking since it updates the type of mkYesodWith to separate the list of contexts from the list of type arguments. This makes mkYesodWith more general since you can now have multiple arguments for contexts. For example:

data Foo a b = Foo a b

class Bar a b where
  ...

mkYesodWith [["Bar", "a", "b"]] "Foo" ["a", "b"] ...

This should generate something like:

instance Bar a b => YesodDispatch (Foo a b) where
    ...

I also updated mkYesod to use the context parser and deprecated mkYesodWith since you should be able to get the same functionality from mkYesod now. Let me know if you don't want to deprecate this.

mkYesodWith was originally introduced in #1055. @Daniel-Diaz, does this look good to you?

This should finish #1365.

After submitting your PR:

  • Bumped the version number
  • 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)

@jprider63 jprider63 changed the base branch from master to better-monads January 24, 2018 18:18
@jprider63
Copy link
Contributor Author

I've changed the target to better-monads. There aren't any conflicts, but I don't think the tests reran.

@Daniel-Diaz
Copy link
Contributor

@jprider63 I'm OK with more flexibility, but maybe it would be good to have a note on how to migrate current code? Or is that not necessary?

@jprider63
Copy link
Contributor Author

I doubt it's necessary since this shouldn't impact the majority of users, but I added some more documentation to mkYesod and mkYesodWith. There's also a more detailed example in the cookbook.

It looks like mkYesodSub was removed, so I changed mkYesod's documentation to point to mkYesodSubData and mkYesodSubDispatch instead.

@Daniel-Diaz
Copy link
Contributor

@jprider63 Well, I do use this function, and I would not assume how many people use it. And even a minority deserves a pleasant migration. :) Anyway, thanks for extending the docs! It looks good to me.

@snoyberg
Copy link
Member

@Daniel-Diaz Just to confirm: are you OK with this change making it into yesod-core 1.6?

@Daniel-Diaz
Copy link
Contributor

@snoyberg Yes.

@snoyberg
Copy link
Member

Cool, thanks!

@snoyberg snoyberg merged commit fe233dd into yesodweb:better-monads Jan 29, 2018
@jprider63
Copy link
Contributor Author

Thanks!

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.

None yet

3 participants