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

Upstream helper functions to Yesod.Form.Option. #1828

Merged
merged 5 commits into from
Feb 27, 2024

Conversation

LukeTemp
Copy link
Contributor

@LukeTemp LukeTemp commented Nov 22, 2023

The 2 new functions are:

  • optionsFromList': creates an OptionList from a List, using the PathPiece instance for the external value and a custom function for the user-facing value.
  • optionsEnum': creates an OptionList from an enumeration.

Also bump the version and update the changelog.

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)

LukeTemp and others added 3 commits November 22, 2023 16:30
The 2 new functions are:
- optionsFromList': creates an OptionList from a List,
using the PathPiece instance for the external value
and a custom function for the user-facing value.
- optionsEnum': creates an OptionList from an
enumeration.

Also bump the version and update the changelog.
@LukeTemp
Copy link
Contributor Author

May I request a review from @snoyberg

Copy link
Member

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

This overall seems like a very specific solution, I'm not sure it belongs in the library.

@jezen
Copy link
Member

jezen commented Dec 8, 2023

I think these are quite generally useful — relying on PathPiece instances is a nice pattern in Yesod.

If these functions are too specific for this library, perhaps they can go into some adjacent library, e.g., create a yesod-form-extra.

In any case, it isn't immediately obvious (especially to someone newer to Yesod) why they might use these functions, so I would like to see example usage documentation, similar to how it's done in the documentation for the persistent library. See here: https://hackage.haskell.org/package/persistent-2.14.6.0/docs/Database-Persist-Class.html#v:get

@snoyberg
Copy link
Member

snoyberg commented Dec 9, 2023

@jezen more than happy to defer to you on the decision of inclusion

LukeTemp and others added 2 commits December 13, 2023 11:09
- Add example usage to optionsFromList'.
- Explain optionsEnum' using optionsFromList'.
- Add @SInCE tags.
@LukeTemp LukeTemp force-pushed the upstream-supercede-form-field-2 branch from bf5d1ff to b1a3b53 Compare December 13, 2023 11:23
Copy link
Member

@jezen jezen 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 good — the examples are really clear. Thank you.

It'd be great to see automated tests, and haddocks for each of the function arguments, but let's go with this as-is for now.

@jezen jezen merged commit 5cfe884 into yesodweb:master Feb 27, 2024
@jezen jezen deleted the upstream-supercede-form-field-2 branch February 27, 2024 14:11
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.

3 participants