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

Provide invocation arguments for RepositoryMethodInvocation #2681

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

roookeee
Copy link

See #2677 for more information about this PR.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 26, 2022
@@ -56,7 +56,7 @@ public void notifyListeners(Method method, Object[] args, RepositoryMethodInvoca

/**
* {@link RepositoryInvocationMulticaster} implementation that notifies {@link RepositoryMethodInvocationListener}
* upon {@link #notifyListeners(Method, Object[], RepositoryMethodInvocationResult)}.
Copy link
Author

Choose a reason for hiding this comment

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

I just fixed this broken JavaDoc which I noticed along the way, it's not directly related to this pr.

@roookeee
Copy link
Author

I put the new arguments parameter where it felt "right", let me know if you want it some other way (e.g. always at the end).
RepositoryInvocationMulticaster could be refactored to not be passed the method and args as they are provided by the RepositoryMethodInvocation - I didn't want to touch this without any further insight from a maintainer.

@@ -197,6 +197,7 @@ void capturesImperativeSuccessCorrectly() throws Exception {

assertThat(multicaster.first().getResult().getState()).isEqualTo(State.SUCCESS);
assertThat(multicaster.first().getResult().getError()).isNull();
assertThat(multicaster.first().getArguments()).isNotNull();
Copy link
Author

Choose a reason for hiding this comment

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

I felt like its appropriate to check all state case results should have arguments present, any thoughts on this? I could make this a more explicit check if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants