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

Add an options data structure to allow fine-tuned control of what instances are generated for a route #1819

Merged

Conversation

Benjamin-McRae-Tracsis
Copy link
Contributor

@Benjamin-McRae-Tracsis Benjamin-McRae-Tracsis commented Sep 13, 2023

Before submitting your PR, check that you've:

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)

Copy link
Contributor

@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.

So that's going to be a breaking change for any library user that uses the Read instance of the type

To avoid a major breaking change, we need to keep the default behavior of deriving Read, but provide an option for the user to disable Read derivation.

@Benjamin-McRae-Tracsis
Copy link
Contributor Author

I can try to work on that.

How would you want this configurable? I can make a mkRouteConsOpt that requires a config data structure, which has three booleans: hasShow, hasEq, hasRead, and then make duplicate versions of all the functions that call mkRouteCons that are suffixed with Opts to take said Opts.

@parsonsmatt
Copy link
Contributor

I'm not terribly particular. I think @snoyberg mentioned using a directive in the actual routes file would be fine - maybe something like:

!no-derive-read

as a very quick, patch-level fix.

Defining an Opts type make sense - I'd prefer a specific name (ie RouteDeriveOpts), and ideally a hidden constructor with setter functions so new fields aren't a breaking change. However, since we don't need to use this to derive instances - users can use StandaloneDeriving to write deriving stock Read (Route App), after all - I don't anticipate many changes to this type, so hiding the constructor and setters would be a matter of taste rather than real pragmaticism.

@Benjamin-McRae-Tracsis
Copy link
Contributor Author

Benjamin-McRae-Tracsis commented Sep 14, 2023

Are there other examples of such directives? I'm looking at the parser and I'm not sure whether there are other directives, and I don't necessarily want to rewrite this parser.

Copy link
Contributor

@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.

PR looks great, thank you! I made some suggestions for the version number + @since comments.

yesod-core/ChangeLog.md Outdated Show resolved Hide resolved
yesod-core/src/Yesod/Core/Internal/TH.hs Outdated Show resolved Hide resolved
yesod-core/src/Yesod/Core/Internal/TH.hs Show resolved Hide resolved
yesod-core/src/Yesod/Core/Internal/TH.hs Show resolved Hide resolved
yesod-core/src/Yesod/Core/Internal/TH.hs Outdated Show resolved Hide resolved
yesod-core/src/Yesod/Routes/TH/RenderRoute.hs Outdated Show resolved Hide resolved
yesod-core/src/Yesod/Routes/TH/RenderRoute.hs Outdated Show resolved Hide resolved
yesod-core/src/Yesod/Routes/TH/RenderRoute.hs Show resolved Hide resolved
yesod-core/src/Yesod/Routes/TH/RenderRoute.hs Show resolved Hide resolved
yesod-core/yesod-core.cabal Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Looks great! Thank you 😄

@Benjamin-McRae-Tracsis Benjamin-McRae-Tracsis changed the title remove read from the list of derived instances Add an options data structure to allow fine-tuned control of what instances are generated for a route Oct 23, 2023
@Benjamin-McRae-Tracsis
Copy link
Contributor Author

Is anything else needed for merging?

@parsonsmatt
Copy link
Contributor

CI failures look spurious. I'll merge now.

@snoyberg I don't have maintainer rights on Hackage for yesod-core. Would you be willing to either add me or do an upload here? Thanks!

@parsonsmatt parsonsmatt merged commit 22c5e46 into yesodweb:master Oct 23, 2023
10 of 14 checks passed
@snoyberg
Copy link
Member

@parsonsmatt absolutely, I'd be happy to. What's your Hackage username?

@parsonsmatt
Copy link
Contributor

It's parsonsmatt there too. Thank you!

@snoyberg
Copy link
Member

@parsonsmatt I've given you maintainer rights on Hackage for yesod-core and a bunch of other Yesod packages too. If you need any more, just let me know. Thank you!

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