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

Added UriInfo#getMatchedResourceTemplate method #1236

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

jansupol
Copy link
Contributor

Implements #1191

There can be a discussion of the actual method name, as the proposed name was getResourceTemplate, but getMatchedResourceTemplate is more aligned with other matched methods.

Also, the return type can be in the form of a list, but the discussion was to return a string

Copy link
Contributor

@jim-krueger jim-krueger left a comment

Choose a reason for hiding this comment

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

Should there be TCK additions to ee.jakarta.tck.ws.rs.ee.rs.core.uriinfo.URIInfoTest?

@jansupol
Copy link
Contributor Author

Should there be TCK additions to ee.jakarta.tck.ws.rs.ee.rs.core.uriinfo.URIInfoTest?

Definitely. I plan to add those if accepted

@jim-krueger
Copy link
Contributor

Would be nice to get this into 4.0. Not sure if there is enough runway.

Added TCK tests

Signed-off-by: jansupol <[email protected]>
@jansupol
Copy link
Contributor Author

Added the TCK tests

@jim-krueger
Copy link
Contributor

With the TCK I am in favor of adding this to the Jakarta Rest 4.0 content. Can we get a quick vote on this?

@ivargrimstad Can we still add this relatively minor, but nice to have, improvement to 4.0 (assuming it passes a vote)?

jim-krueger
jim-krueger previously approved these changes Mar 25, 2024
@jansupol
Copy link
Contributor Author

An alternative with returning the List of templates #1245

@jansupol
Copy link
Contributor Author

I slightly prefer the #1245 over this one

Copy link
Contributor

@jamezp jamezp left a comment

Choose a reason for hiding this comment

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

In talking with some of the OTel folks, they seemed to prefer the string return type so they don't have to combine the list to a string.

@jim-krueger
Copy link
Contributor

jim-krueger commented Mar 27, 2024

Yes, this was run past Andrew, the originator of the issue and he concurred. So this is the PR that should be merged.

@spericas spericas merged commit 7c34889 into jakartaee:main Mar 28, 2024
3 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.

4 participants