Skip to content

Remove unused method parameters #25986

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

Conversation

dreis2211
Copy link
Contributor

Hi,

this PR removes a couple unused method parameters.

Cheers,
Christoph

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 9, 2021
@dreis2211
Copy link
Contributor Author

Test failure seems unrelated.

@philwebb
Copy link
Member

philwebb commented Apr 9, 2021

Nice!

@philwebb philwebb added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 9, 2021
@philwebb philwebb added this to the 2.5.x milestone Apr 9, 2021
@wilkinsona
Copy link
Member

Test failure seems unrelated

I've just seen the same failure. I believe it's due to this change in Spring Security. I've reached out to the Security team for some guidance as I'm not sure if the change is intentional and if we need to add spring-security-web to the smoke test's dependencies.

@wilkinsona
Copy link
Member

The spring-security-web dependency has been reinstated.

Comment on lines +104 to 110
private void onApplicationPreparedEvent() {
finish();
}

private void onApplicationFailedEvent(ApplicationFailedEvent event) {
private void onApplicationFailedEvent() {
finish();
}
Copy link

@cuspymd cuspymd Apr 10, 2021

Choose a reason for hiding this comment

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

It seems a bit ambiguous to call it a handler function for the event by deleting the event parameter.
Wouldn't it be better to get rid of the wrapper function and call finish() directly in onApplicationEvent()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about that actually, but I liked the clear separation of things here. For me they are still the containers for the logic that should happen in case of the respective event and just share the common finish() call - regardless of the fact that they don't need the parameter. But I have no strong feelings for either option. Let's see what the team thinks and I'll gladly change to whatever is preferred.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer it as currently proposed.

Copy link

Choose a reason for hiding this comment

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

Thanks for the considering.

@snicoll snicoll self-assigned this Apr 12, 2021
@snicoll snicoll modified the milestones: 2.5.x, 2.5.0-RC1 Apr 12, 2021
snicoll pushed a commit that referenced this pull request Apr 12, 2021
snicoll added a commit that referenced this pull request Apr 12, 2021
@snicoll snicoll closed this in b9bfc15 Apr 12, 2021
humaolin pushed a commit to humaolin/spring-boot that referenced this pull request May 7, 2022
humaolin pushed a commit to humaolin/spring-boot that referenced this pull request May 7, 2022
humaolin pushed a commit to humaolin/spring-boot that referenced this pull request May 7, 2022
humaolin pushed a commit to humaolin/spring-boot that referenced this pull request May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants