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

Fix CaptureAll and support for RawM WithResource #11

Merged

Conversation

HanStolpo
Copy link

We fix the implementation of the CaptureAll instance. Previously paths would never be matched since the instance did not consume the rest of the path like CaptureAll does. The rest of the path is now captured and replaced with a * place holder and this is also the case for enumerating the endpoint.

We also add instances for RawM and WithResource and add a test case to the spec to check that CaptureAll and RawM behave as expected.

@HanStolpo HanStolpo force-pushed the fix-capture-all-rawm-withresource branch from 152a9e0 to d7f3ff8 Compare November 25, 2024 10:20
@HanStolpo
Copy link
Author

The tests run successfully locally with

  • GHC 9.8.2
  • servant-0.20.2

@worm2fed
Copy link
Owner

Hi @HanStolpo thank you for your PR. Loogs good for me, but tests in CI are failing. Could you please fix it?

@HanStolpo
Copy link
Author

Sure I will fix it, I just am not sure what the actual failure in CI is, it looks like its stack related but I didn't change anything in the stack config, ah but I guess the cabal file gets generated from the stack.yaml.

@worm2fed
Copy link
Owner

worm2fed commented Nov 25, 2024

@HanStolpo yes, you should change stack (package.yaml) instead of cabal)

@HanStolpo
Copy link
Author

I looks like I will have to add some CPP to support different servant versions I think one of or both RawM and WithResource might not be in all the supported servant versions

@worm2fed
Copy link
Owner

Probably yes
I didn't used this for more then a year, so a bit out of context

@HanStolpo
Copy link
Author

Okay yes I see RawM and WithResource is only from 0.20

We fix the implementation of the CaptureAll instance. Previously paths
would never be matched since the instance did not consume the rest of
the path like `CaptureAll` does. The rest of the path is now captured
and replaced with a `*` place holder and this is also the case for
enumerating the endpoint.

We also add instances for `RawM` and `WithResource` and add a test case
to the spec to check that `CaptureAll` and `RawM` behave as expected.
@HanStolpo HanStolpo force-pushed the fix-capture-all-rawm-withresource branch from e2ec881 to b148b3b Compare November 25, 2024 12:54
@HanStolpo
Copy link
Author

Okay I generated the cabal file from the updated package.yaml file and version gated the RawM and WithResource instances, I also changed the tests to use Raw instead of RawM for simplicity.

@HanStolpo
Copy link
Author

Sorry the version gating is not quite correct

@HanStolpo HanStolpo force-pushed the fix-capture-all-rawm-withresource branch from b148b3b to 7faa7c2 Compare November 25, 2024 13:03
@HanStolpo
Copy link
Author

Okay I fixed the version gating. It might be useful to also run tests with later GHCs / Servant versions. I didn't look at the workflow stuff though.

@worm2fed
Copy link
Owner

@HanStolpo actually, previously it was correct... no there is 0.2.0
Here is an example)

#if MIN_VERSION_servant(0,18,2)

@worm2fed
Copy link
Owner

yes, that'll be good to launch with matrix, maybe will add this later

@worm2fed worm2fed merged commit 9f967b3 into worm2fed:master Nov 26, 2024
1 check 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.

2 participants