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

Renamed AtLeastOneFragment type class to AtMostOneFragment #1727

Merged
merged 4 commits into from
Mar 16, 2024

Conversation

DavidMazarro
Copy link
Contributor

@DavidMazarro DavidMazarro commented Mar 9, 2024

Unless I'm misunderstanding the FragmentUnique type family, it's checking that there's at most one fragment in the API type. In that case, the AtLeastOneFragment name is not only confusing, but potentially misleading. This PR renames the type class to AtMostOneFragment.

In addition, I did some small changes to the documentation, I moved the explanation of the type class to the type class definition (previously the comment was under the Verb instance) and reworded it a bit.

@DavidMazarro DavidMazarro changed the title Renamed AtLeastOneFragment to AtMostOneFragment Renamed AtLeastOneFragment type class to AtMostOneFragment Mar 9, 2024
@tchoutri
Copy link
Contributor

tchoutri commented Mar 9, 2024

@jkarni would you mind giving your opinion on this?

@tchoutri
Copy link
Contributor

tchoutri commented Mar 9, 2024

@DavidMazarro Thanks for the PR!

@tchoutri tchoutri self-assigned this Mar 9, 2024
Copy link
Member

@jkarni jkarni left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@DavidMazarro
Copy link
Contributor Author

(I added these last two commits because I was unfamiliar with doctest and didn't know the code examples were checked, my bad!)

@DavidMazarro
Copy link
Contributor Author

Feel free to run the CI on this one again, but other than that it's ready to merge from my side :)

@DavidMazarro
Copy link
Contributor Author

@tchoutri could you run the CI workflow again?

@tchoutri
Copy link
Contributor

Alright, I have looked up the usage of AtLeastOneFragment across openn-source codebases on github and hackage, looks like we can get away with this breaking change.

Thanks @DavidMazarro!

@tchoutri tchoutri merged commit ef4b38a into haskell-servant:master Mar 16, 2024
8 checks passed
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