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.

Sign up for GitHub

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

Jump to bottom

More precise language for InvocationContext.getInterceptorBindings() #103

Merged
merged 1 commit into from Nov 30, 2023
Merged

More precise language for InvocationContext.getInterceptorBindings() #103

merged 1 commit into from Nov 30, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jul 18, 2023

No description provided.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 18, 2023

When I was working on the TCK tests for InvocationContext.getInterceptorBindings() (jakartaee/cdi-tck#479), I realized there are some "interesting" cases that the previous specification didn't exactly cover well (allowed multiple interpretations). In this PR, I'm trying to tighten up the language to be precise.

manovotn
manovotn approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

LGTM

@manovotn
Copy link
Contributor

Also CC @arjantijms for committer review :)

@arjantijms
Copy link
Contributor

LGTM, I do wonder if the specification text is still necessary when the javadoc already covers it. Though that's maybe a totally different discussion.

@arjantijms arjantijms added this to the 2.2.0 milestone Jul 18, 2023
@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 18, 2023

The reason why I basically copy the javadoc to the specification text is that the specification text should be complete. I generally like AtInject's approach that there is no other specification text than javadoc, but here, we have an extra specification text, and I feel like the best approach is to just duplicate the text.

@Ladicek Ladicek mentioned this pull request Jul 18, 2023
Merged
@manovotn
Copy link
Contributor

@arjantijms is there anything blocking this from getting merged?

@Ladicek Ladicek mentioned this pull request Nov 30, 2023
Merged
@starksm64 starksm64 merged commit 1f68a6b into jakartaee:master Nov 30, 2023
2 checks passed
@Ladicek Ladicek deleted the interceptor-bindings-improved branch November 30, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arjantijms arjantijms arjantijms approved these changes

@manovotn manovotn manovotn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Ladicek @manovotn @arjantijms @starksm64