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

Generalise SegmentResult #1079

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

alexfmpe
Copy link
Contributor

@alexfmpe alexfmpe commented May 15, 2024

Ran into a non-urls usecase where I wanted to use ([Text], ByteString) instead of ([Text], Map Text (Maybe Text)) and realized the groundwork for #989 allowed me to use ([Text], q) as long as I relaxed the type signatures so I pulled that bit into this separate, more conservative PR.

The tutorial describes the PageName version of pathComponentEncoder. Should that be updated with this PR or do we keep it simple?

I have:

  • Based work on latest develop branch
  • Followed the contribution guide
  • Looked for lint in my changes with hlint . (lint found code you did not write can be left alone)
  • Run the test suite: $(nix-build -A selftest --no-out-link)
  • Updated the changelog
  • (Optional) Run CI tests locally: nix-build release.nix -A build.x86_64-linux --no-out-link (or x86_64-darwin on macOS)

@@ -587,11 +587,11 @@ checkEnum1EncoderFunc f = do

-- | This type is used by pathComponentEncoder to allow the user to indicate how to treat
-- various cases when encoding a dependent sum of type `(R p)`.
data SegmentResult check parse a =
PathEnd (Encoder check parse a (Map Text (Maybe Text)))
data SegmentResult check parse a b =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking change - migration while keeping old meaning is to use (Map Text (Maybe Text)) for b, but sometimes this can be generalized, as with indexOnlyRouteSegment.

Do we want to do a proper warn-first-break-later migration cycle at the cost of polluting the namespace with a SegmentResult' or so for a few releases?

@alexfmpe
Copy link
Contributor Author

alexfmpe commented May 15, 2024

Ended up also locally defining a variant of queryOnlyEncoder with the more general type.

I wonder if this should be more of a generalise-query-type thing.

In theory we could do the same for path, so instead of PageName everywhere, unless url-specific we'd instead have ([fragment], query) but I don't actually have a use-case for that so hard to tell what exactly to generalise to. What even is a generic path afterall?

At least allowing () locally as the query type seems pretty convenient and common

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.

1 participant