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 otherParameters to ContentTypeRange, update MediaType#matches to take into account otherParameters. #306

Merged
merged 4 commits into from
Oct 16, 2023

Conversation

kamilkloch
Copy link

Fixes #305.

… to take into account `otherParameters`.
@adamw
Copy link
Member

adamw commented Oct 11, 2023

Could you also create a PR (relying on these changes) in tapir, so that we could review both together (it would necessarily use a locally-published model version)?

@kamilkloch
Copy link
Author

I checked tapir locally against the locally published sttp-model. What do you mean by submitting this as a PR? It will not compile (missing locally published sttp-model).

@adamw
Copy link
Member

adamw commented Oct 11, 2023

I know, but some more changes are needed on the tapir side, no? :) I can then checkout both PRs locally & test

@kamilkloch
Copy link
Author

I did not need to alter a single line in tapir to make it compile, I beileve it should work as expected with the new sttp-model. I tried to add some tests, but got lost in the tapir testing machinery :/ (ContentNegitiation, ServerContentNegotiationTests). I would appreciate your help here. Thank you!

@adamw
Copy link
Member

adamw commented Oct 13, 2023

ServerContentNegotiationTests is the right place to add a test :) If you add a test there, it will be ran when the tests for any specific interpreter's tests are run, e.g. NettyFutureServerTest.

The test itself consists of an endpoint (here you would need to create an endpoitn with the oneOf) and an sttp-client request to make (here you would need to include the appropriate header).

kamilkloch added a commit to kamilkloch/tapir that referenced this pull request Oct 14, 2023
@kamilkloch
Copy link
Author

kamilkloch commented Oct 14, 2023

@adamw please give it a spin: softwaremill/tapir#3243

@adamw adamw merged commit 1202efe into softwaremill:master Oct 16, 2023
4 checks passed
@adamw
Copy link
Member

adamw commented Oct 16, 2023

Thanks for the work :)

@adamw
Copy link
Member

adamw commented Oct 16, 2023

1.7.3 should be out soon

@kamilkloch
Copy link
Author

kamilkloch commented Oct 16, 2023

Once 1.7.3 lands in maven, shall I make a PR to tapir, with the bumped dependency and the added tests?

@adamw
Copy link
Member

adamw commented Oct 16, 2023

Yeah maybe simply update the PR that is already there with the test?

kamilkloch added a commit to kamilkloch/tapir that referenced this pull request Oct 16, 2023
adamw added a commit to softwaremill/tapir that referenced this pull request Oct 17, 2023
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.

MediaType#matches ignores parameters.
2 participants