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

[Javadoc] CSVFormat#setHeaderComments() #344

Merged
merged 8 commits into from
Aug 25, 2023

Conversation

gbidsilva
Copy link
Contributor

Adding one simple detail to make method 'setHeaderComments' doc comment much cleaner.

@garydgregory
Copy link
Member

It seems to me that the get and set methods for this attribute should be more in sync.

@gbidsilva
Copy link
Contributor Author

gbidsilva commented Aug 25, 2023

@garydgregory
Thank you for your quick response on this.
I think I can unerstand what you are trying to explain here.

Currently in the CSVFormat.getHeaderComments() has the following method comment.

Gets a copy of the header comment array.
Returns: a copy of the header comment array; null if disabled.

And if we see the set method, currently it is having the following comment set.

Sets the header comments set to the given values. The comments will be printed first, before the headers. This setting is ignored by the parser.

This seems more in sync. :)

However, the reason for this suggestion is mentioned in Apache Jira: https://issues.apache.org/jira/browse/CSV-308
Could you please have a look at: CSV-308

@garydgregory
Copy link
Member

@gbidsilva
Your new comment is incorrect because it gives the impression that the comment marker is printed once when, in fact, it is printed once per comment element in the array, at the start of each line. This is a case where providing an example or template might be better than trying to use words to describe a file format in prose ;-)

@gbidsilva
Copy link
Contributor Author

@garydgregory
Thank you for pointing it out.
Since I am new to Apache commitments, I am not sure what you were referring to as "providing an example or template might be better".
Do we have a place or an Apache driven articles where we can contribute in this case?
Also I am not clear about the 'template' part.
However we could try changing the comment to be more meaningful and correct if changing comment is more appropriate and way to proceed on this.

@garydgregory
Copy link
Member

What I mean is something like what I just committed:

    /**
     * Gets the character marking the start of a line comment.
     * <p>
     * The comment format for each line is:
     * </p>
     * <pre>
     * CommentMarker SPACE CommentArrayElement
     * </pre>
     * <p>
     * For example, using a comment marker {@code '#'} and a comment array {@code comments ["line 1", "line 2"]}:
     * </p>
     * <pre>
     * # line 1
     * # line 2
     * </pre>
     *
     * @return the comment start marker, may be {@code null}
     */
    public Character getCommentMarker() {
        return commentMarker;
    }

@gbidsilva
Copy link
Contributor Author

@garydgregory
Great help!
Should change the PR to match something like that for setHeaderComments ?

@gbidsilva
Copy link
Contributor Author

@garydgregory : I have updated the PR with suggested changes.

@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2023

Codecov Report

Merging #344 (f1294a7) into master (9668d46) will not change coverage.
Report is 3 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #344   +/-   ##
=========================================
  Coverage     97.87%   97.87%           
  Complexity      549      549           
=========================================
  Files            11       11           
  Lines          1178     1178           
  Branches        204      204           
=========================================
  Hits           1153     1153           
  Misses           13       13           
  Partials         12       12           
Files Changed Coverage Δ
...rc/main/java/org/apache/commons/csv/CSVFormat.java 98.02% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

The example for the String[] API was wrong, it should pass only Strings, not a String and an Instant.
@garydgregory garydgregory changed the title Adding Java doc optimization for method setHeaderComments() in CSVFormat class [Javadoc] CSVFormat#setHeaderComments() Aug 25, 2023
@garydgregory garydgregory merged commit 082827c into apache:master Aug 25, 2023
6 checks passed
@gbidsilva
Copy link
Contributor Author

@garydgregory: Since this PR is merged I think we can safely close the issue: https://issues.apache.org/jira/browse/CSV-308
Thank you all for your support and guidance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants