Skip to content

Document intention of toString() in HandlerMethod #35055

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

Merged

Conversation

wonyongg
Copy link
Contributor

The formatErrorForReturnValue method in ServletInvocableHandlerMethod previously included an unnecessary call to toString() and a dangling "in" at the end of the formatted string.

Since the surrounding context already provides sufficient information, both the toString() call and the " in" suffix have been removed to improve clarity.

No functional changes.

The `formatErrorForReturnValue` method in `ServletInvocableHandlerMethod` previously included an unnecessary call to `toString()` and a dangling "in" at the end of the formatted string.

Since the surrounding context already provides sufficient information, both the `toString()` call and the `" in"` suffix have been removed to improve clarity.

No functional changes.

Signed-off-by: wonyongg <[email protected]>
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 15, 2025
@bclozel bclozel added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jun 15, 2025
@wonyongg
Copy link
Contributor Author

After revisiting the HandlerMethod implementation, I realized that I misunderstood the original purpose of the toString() call.

Initially, I thought the usage of toString() was redundant or unclear. However, I now understand that it returns the description string, which actually provides useful context like ClassName#methodName(args...). sorry for the confusion.

That said, I'd like to propose a small improvement.

The method name toString() by itself doesn't make the intention very clear to someone unfamiliar with the HandlerMethod class. I believe replacing it with getShortLogMessage() could make the code more readable and self-explanatory, especially for contributors or maintainers not deeply familiar with this class.

The output remains similar, but the method name communicates its purpose more directly.

If this suggestion doesn't align with the direction of the project, please feel free to suggest an alternative approach or close this PR if it's unnecessary.

@wonyongg wonyongg changed the title Remove redundant toString() call and trailing "in" from error message Clarify the intention of toString() usage in error logging Jun 15, 2025
@sbrannen
Copy link
Member

sbrannen commented Jun 16, 2025

The method name toString() by itself doesn't make the intention very clear to someone unfamiliar with the HandlerMethod class. I believe replacing it with getShortLogMessage() could make the code more readable and self-explanatory, especially for contributors or maintainers not deeply familiar with this class.

The output remains similar, but the method name communicates its purpose more directly.

If this suggestion doesn't align with the direction of the project, please feel free to suggest an alternative approach or close this PR if it's unnecessary.

I'm not so sure that it's worth changing things here.

If we were to change something, I suppose we could either make the HandlerMethod.description field protected or introduce a getDescription() method in HandlerMethod.

We'll discuss it within the team and get back to you.

@sbrannen sbrannen marked this pull request as draft June 16, 2025 14:12
@sbrannen sbrannen changed the title Clarify the intention of toString() usage in error logging Clarify intention of toString() usage in log messages in HandlerMethod Jun 16, 2025
@sbrannen sbrannen self-assigned this Jun 16, 2025
@wonyongg
Copy link
Contributor Author

@sbrannen
Thank you for your feedback! I’ll be waiting to hear back after your team discussion.

@sbrannen
Copy link
Member

Instead of changing the code or introducing a dedicated method to retrieve the description, I think we should just update the Javadoc for HandlerMethod.toString() to document that it's used in log/error messages and should typically include the method signature of the handler method.

If you would like to repurpose this PR to do that, feel free to do so.

Otherwise, I'll just add the Javadoc myself.

@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Jun 18, 2025
Document that HandlerMethod.toString() is used in log and error messages,
and that the returned description should typically include the method
signature of the underlying handler method for clarity and debugging.

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

Thanks for the feedback! 🙏
I’ve removed the previous changes and updated this PR to only include the Javadoc for HandlerMethod.toString() as suggested.
I’d appreciate it if you could take a quick look and let me know if the updated Javadoc looks good to you.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 18, 2025
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

Thanks for the proposed Javadoc.

I've requested minor changes.

@sbrannen sbrannen added status: waiting-for-feedback We need additional information before we can continue type: documentation A documentation task and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jun 18, 2025
@sbrannen sbrannen added this to the 7.0.0-M7 milestone Jun 18, 2025
@sbrannen sbrannen changed the title Clarify intention of toString() usage in log messages in HandlerMethod Document intention of toString() in HandlerMethod Jun 18, 2025
…rMethod.java

Co-authored-by: Sam Brannen <[email protected]>
Signed-off-by: WonYong Hwang <[email protected]>
@sbrannen sbrannen removed the status: waiting-for-feedback We need additional information before we can continue label Jun 18, 2025
@sbrannen sbrannen marked this pull request as ready for review June 18, 2025 10:59
@sbrannen sbrannen merged commit 5d0fc72 into spring-projects:main Jun 18, 2025
1 check passed
@sbrannen
Copy link
Member

This has been merged into main.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants