Skip to content

Improve JavaDoc for toStringArray methods #1417

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

Closed

Conversation

Naveenkumarsuk
Copy link

Updated JavaDoc for both overloaded toStringArray methods in ArrayUtils.java to be clearer and more consistent.
This improves readability and clarity for developers using these methods.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the JavaDoc documentation for two overloaded toStringArray methods in the Apache Commons Lang ArrayUtils class. The changes focus on making the documentation more concise and consistent between the two method variants.

  • Simplified and clarified method descriptions
  • Made parameter and return value documentation more consistent
  • Removed redundant information and verbose explanations
Comments suppressed due to low confidence (2)

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

Hi @Naveenkumarsuk,

Thanks for the PR and for taking the time to improve the Javadoc!

It is mostly a matter of opinion, others might feel differently, but your updated descriptions seem more approachable to me. However, there are a few issues to address before this can be merged:

  • Nullability Regression: The @param tag for the nullable argument previously indicated that null was allowed. This is an important part of the contract, and it's helpful to preserve that information explicitly in the Javadoc.
  • Javadoc Formatting: There are some indentation issues.
  • Minor Style Point: While HTML allows omitting closing </p> tags, including them tends to improve readability and consistency across the codebase. I'd recommend keeping them in.

@Naveenkumarsuk Naveenkumarsuk force-pushed the fix-tostringarray-javadoc branch from 5a82b1d to 3c70bff Compare July 20, 2025 07:39
@Naveenkumarsuk Naveenkumarsuk force-pushed the fix-tostringarray-javadoc branch from edcf48a to 871b2b4 Compare July 20, 2025 08:36
@Naveenkumarsuk
Copy link
Author

Hi @ppkarwasz Thanks again for the detailed feedback. I've updated the Javadoc as suggested:

Restored clear null-handling info in both summary and tags
Closed all

tags properly.
Adjusted formatting and spacing.
Please let me know if any other improvements are needed.
Thank You

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

@Naveenkumarsuk,

Force pushes make reviewing harder (you need to review from the start). This is not a big problem here, but could you avoid force pushing?

@garydgregory
Copy link
Member

This makes the docs worse with random looking changes. Is this from a bot?

@ppkarwasz
Copy link
Contributor

This makes the docs worse with random looking changes. Is this from a bot?

The lack of indentation suggests that Copilot was involved somewhere in the process: I don't know any IDE that formats Javadoc like this, but if I ask Copilot to do it for me, that is the format I get.

However, although AI-aided, I guess this is a genuine PR.

@garydgregory
Copy link
Member

garydgregory commented Jul 20, 2025

@ppkarwasz
The fact that two maintainers are cycling on this is a very bad sign IMO. We should close this PR and if you feel the Javadoc can be improved, you can just do it instead of dealing with AI slop.

@ppkarwasz
Copy link
Contributor

@garydgregory,

Since I use AI too for many non-critical tasks (e.g. generate some documentation without wasting my time), I am not so adamant against AI-generated content. As long as a human controls the process, AI is just a tool!

@Naveenkumarsuk,

That said, Gary is partially right: if you are using AI as a tool, please correct its output. What we expect is:

  • Having the same documentation on both the methods: in the end one of them is a specialization of the other with "null" as second parameter.
  • Correct indentation.
  • Correct @since documentation: this is very useful when somebody looks at the published Javadoc, but does not find the method in its own application.
  • Nullability documentation of the @param tags: this is part of the contract of the method and can not be removed.

@Naveenkumarsuk
Copy link
Author

Naveenkumarsuk commented Jul 20, 2025

@ppkarwasz Thank You for the detailed feedback.

Just to be fully transparent I did use GitHub Copilot to assist with this contribution, mainly for generating initial drafts of Javadoc and code structure. However, I reviewed and edited everything myself to ensure it aligns with the style, correctness, and expectations of the project. I treat AI as a productivity tool, not as a substitute for understanding or quality control. Let me know if there are any specific standards you'd like me to follow when using such tools. Used bash and double checked if there are any errors,and the build was success locally. I’ll go ahead and:

Ensure the Javadoc is consistent between both method variants,
Fix the indentation issues,
Add the correct @SInCE tag,
Restore and complete the nullability information for the @param tags.

I’ll push the updates shortly as a regular commit (not a force push) to keep the review history clean.
Appreciate the helpful and specific feedback.

TY

@Naveenkumarsuk Naveenkumarsuk requested a review from ppkarwasz July 20, 2025 16:12
@Naveenkumarsuk
Copy link
Author

@ppkarwasz I've updated the JavaDocs as requested. Please close the PR if it still fails. I am working also for my own Startup and few other projects and I am interested in contributing in my free times,that's why i joined Github.

TY in advance.

@ppkarwasz
Copy link
Contributor

@Naveenkumarsuk, you didn't push any new commits to your PR branch.

Comment on lines +9540 to +9541
*/
public static String[] toStringArray(final Object[] array) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you notice that instead of indenting the Javadoc 4 spaces, you did deindent the code?

public static String[] toStringArray(final Object[] array) {
return toStringArray(array, "null");
/**
* Converts an array of objects to an array of strings, handling {@code null} values gracefully.
Copy link
Member

Choose a reason for hiding this comment

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

I think I am going towards -1 because this looks to me like change for the sake of change. Why call it "gracefully"? Should we document when an implementation is not garceful?

@ppkarwasz
Copy link
Contributor

@Naveenkumarsuk,

Thank you for your contribution and for engaging constructively throughout the review process.

While your intention to improve the documentation is appreciated, this PR introduced several regressions — including the removal of important nullability annotations, formatting inconsistencies, and the omission of the @since tag — which outweigh the benefits of the proposed changes. Additionally, the most recent commit 1ce9413 unexpectedly modified the method implementation itself, which was not discussed or justified in the original scope of the PR.

Given these concerns and the unclear overall value added by the changes, I’m closing this PR. We appreciate your effort, and we encourage future contributions — especially if they remain narrowly scoped, well-reviewed, and aligned with the project's established conventions.

Thanks again.

@ppkarwasz ppkarwasz closed this Jul 20, 2025
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.

3 participants