You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "jkbkupczyk (via GitHub)" <gi...@apache.org> on 2023/07/04 20:53:14 UTC
[GitHub] [commons-net] jkbkupczyk opened a new pull request, #162: Add FTPFile tests and fix javadoc typos
jkbkupczyk opened a new pull request, #162:
URL: https://github.com/apache/commons-net/pull/162
- Fixed some typos in Javadoc and adjusted documentation to code
- Replaced HTML snippets with Javadoc counterparts
- Moved serialization-related methods under the "serialization comment"
- Adds FTPFile tests
FYI @kinow @garydgregory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on a diff in pull request #162: Add FTPFile tests and fix Javadoc typos
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1253099624
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -466,6 +467,9 @@ public String toFormattedString(final String timezone) {
/**
* Gets a string representation of the FTPFile information.
+ * The returned output will be an FTP server raw listing.
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on a diff in pull request #162: Add FTPFile tests and fix javadoc typos
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1252994889
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -330,13 +330,14 @@ public void setName(final String name) {
}
/**
- * Sets if the given access group (one of the <code> _ACCESS </code> constants) has the given access permission (one of the <code> _PERMISSION </code>
+ * Sets if the given access group (one of the {@code _ACCESS} constants) has the given access permission (one of the {@code _PERMISSION}
* constants) to the file.
*
- * @param access The access group (one of the <code> _ACCESS </code> constants)
- * @param permission The access permission (one of the <code> _PERMISSION </code> constants)
- * @param value True if permission is allowed, false if not.
+ * @param access The access group (one of the {@code _ACCESS} constants)
+ * @param permission The access permission (one of the {@code _PERMISSION} constants)
+ * @param value {@code true} if permission is allowed, {@code false} if not.
* @throws ArrayIndexOutOfBoundsException if either of the parameters is out of range
+ * @throws NullPointerException if you're trying to set permission of an invalid file
Review Comment:
[FTPListParseEngine](https://github.com/apache/commons-net/blob/master/src/main/java/org/apache/commons/net/ftp/FTPListParseEngine.java) will use `FTPFile(String)` constructor (for invalid files) that sets `permissions` to `null`;
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory commented on a diff in pull request #162: Add FTPFile tests and fix javadoc typos
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1252907537
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -330,13 +330,14 @@ public void setName(final String name) {
}
/**
- * Sets if the given access group (one of the <code> _ACCESS </code> constants) has the given access permission (one of the <code> _PERMISSION </code>
+ * Sets if the given access group (one of the {@code _ACCESS} constants) has the given access permission (one of the {@code _PERMISSION}
* constants) to the file.
*
- * @param access The access group (one of the <code> _ACCESS </code> constants)
- * @param permission The access permission (one of the <code> _PERMISSION </code> constants)
- * @param value True if permission is allowed, false if not.
+ * @param access The access group (one of the {@code _ACCESS} constants)
+ * @param permission The access permission (one of the {@code _PERMISSION} constants)
+ * @param value {@code true} if permission is allowed, {@code false} if not.
* @throws ArrayIndexOutOfBoundsException if either of the parameters is out of range
+ * @throws NullPointerException if you're trying to set permission of an invalid file
Review Comment:
How can an NPE happen here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory commented on a diff in pull request #162: Add FTPFile tests and fix javadoc typos
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1252911449
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -466,6 +467,9 @@ public String toFormattedString(final String timezone) {
/**
* Gets a string representation of the FTPFile information.
+ * The returned output will be an FTP server raw listing.
Review Comment:
The whole Javadoc can just say "Delegates to ..." and use a link tag to refer to the method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory commented on a diff in pull request #162: Add FTPFile tests and fix javadoc typos
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1252908213
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -466,6 +467,9 @@ public String toFormattedString(final String timezone) {
/**
* Gets a string representation of the FTPFile information.
+ * The returned output will be an FTP server raw listing.
+ *
+ * @see #getRawListing()
*
Review Comment:
No blank line between tags.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on a diff in pull request #162: Add FTPFile tests and fix Javadoc typos
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1253103689
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -466,6 +467,9 @@ public String toFormattedString(final String timezone) {
/**
* Gets a string representation of the FTPFile information.
+ * The returned output will be an FTP server raw listing.
+ *
+ * @see #getRawListing()
*
Review Comment:
done, I also removed the blank line in `toFormattedString` Javadoc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory commented on a diff in pull request #162: Add FTPFile tests and fix Javadoc typos
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1253053996
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -330,13 +330,14 @@ public void setName(final String name) {
}
/**
- * Sets if the given access group (one of the <code> _ACCESS </code> constants) has the given access permission (one of the <code> _PERMISSION </code>
+ * Sets if the given access group (one of the {@code _ACCESS} constants) has the given access permission (one of the {@code _PERMISSION}
* constants) to the file.
*
- * @param access The access group (one of the <code> _ACCESS </code> constants)
- * @param permission The access permission (one of the <code> _PERMISSION </code> constants)
- * @param value True if permission is allowed, false if not.
+ * @param access The access group (one of the {@code _ACCESS} constants)
+ * @param permission The access permission (one of the {@code _PERMISSION} constants)
+ * @param value {@code true} if permission is allowed, {@code false} if not.
* @throws ArrayIndexOutOfBoundsException if either of the parameters is out of range
+ * @throws NullPointerException if you're trying to set permission of an invalid file
Review Comment:
That's not a "feature" and should not be documented but fixed in a different PR or commit. IOW, if null input is illegal it should be made so in a setter or ctor IMO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory commented on a diff in pull request #162: Add FTPFile tests and fix Javadoc typos
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1253053996
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -330,13 +330,14 @@ public void setName(final String name) {
}
/**
- * Sets if the given access group (one of the <code> _ACCESS </code> constants) has the given access permission (one of the <code> _PERMISSION </code>
+ * Sets if the given access group (one of the {@code _ACCESS} constants) has the given access permission (one of the {@code _PERMISSION}
* constants) to the file.
*
- * @param access The access group (one of the <code> _ACCESS </code> constants)
- * @param permission The access permission (one of the <code> _PERMISSION </code> constants)
- * @param value True if permission is allowed, false if not.
+ * @param access The access group (one of the {@code _ACCESS} constants)
+ * @param permission The access permission (one of the {@code _PERMISSION} constants)
+ * @param value {@code true} if permission is allowed, {@code false} if not.
* @throws ArrayIndexOutOfBoundsException if either of the parameters is out of range
+ * @throws NullPointerException if you're trying to set permission of an invalid file
Review Comment:
That's not a "feature" and should not be documented but fixed in a different PR or commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on a diff in pull request #162: Add FTPFile tests and fix Javadoc typos
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1253112065
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -330,13 +330,14 @@ public void setName(final String name) {
}
/**
- * Sets if the given access group (one of the <code> _ACCESS </code> constants) has the given access permission (one of the <code> _PERMISSION </code>
+ * Sets if the given access group (one of the {@code _ACCESS} constants) has the given access permission (one of the {@code _PERMISSION}
* constants) to the file.
*
- * @param access The access group (one of the <code> _ACCESS </code> constants)
- * @param permission The access permission (one of the <code> _PERMISSION </code> constants)
- * @param value True if permission is allowed, false if not.
+ * @param access The access group (one of the {@code _ACCESS} constants)
+ * @param permission The access permission (one of the {@code _PERMISSION} constants)
+ * @param value {@code true} if permission is allowed, {@code false} if not.
* @throws ArrayIndexOutOfBoundsException if either of the parameters is out of range
+ * @throws NullPointerException if you're trying to set permission of an invalid file
Review Comment:
done,
removed _throws_ tag and added _todo_ message
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory merged pull request #162: Add FTPFile tests and fix Javadoc typos
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #162:
URL: https://github.com/apache/commons-net/pull/162
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on a diff in pull request #162: Add FTPFile tests and fix javadoc typos
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on code in PR #162:
URL: https://github.com/apache/commons-net/pull/162#discussion_r1252994889
##########
src/main/java/org/apache/commons/net/ftp/FTPFile.java:
##########
@@ -330,13 +330,14 @@ public void setName(final String name) {
}
/**
- * Sets if the given access group (one of the <code> _ACCESS </code> constants) has the given access permission (one of the <code> _PERMISSION </code>
+ * Sets if the given access group (one of the {@code _ACCESS} constants) has the given access permission (one of the {@code _PERMISSION}
* constants) to the file.
*
- * @param access The access group (one of the <code> _ACCESS </code> constants)
- * @param permission The access permission (one of the <code> _PERMISSION </code> constants)
- * @param value True if permission is allowed, false if not.
+ * @param access The access group (one of the {@code _ACCESS} constants)
+ * @param permission The access permission (one of the {@code _PERMISSION} constants)
+ * @param value {@code true} if permission is allowed, {@code false} if not.
* @throws ArrayIndexOutOfBoundsException if either of the parameters is out of range
+ * @throws NullPointerException if you're trying to set permission of an invalid file
Review Comment:
[FTPListParseEngine](https://github.com/apache/commons-net/blob/master/src/main/java/org/apache/commons/net/ftp/FTPListParseEngine.java) will use `FTPFile(String)` constructor that sets `permissions` to `null`;
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory commented on pull request #162: Add FTPFile tests and fix javadoc typos
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #162:
URL: https://github.com/apache/commons-net/pull/162#issuecomment-1620783982
Hi @jkbkupczyk
Please don't move methods in PRs, it makes your PR noisier, and takes longer to review. The methods were sorted in AB order, let's keep it that way, nice and simple, easier to find code, especially in PRs ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on pull request #162: Add FTPFile tests and fix javadoc typos
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on PR #162:
URL: https://github.com/apache/commons-net/pull/162#issuecomment-1621046327
@garydgregory done
Thought it looked kinda odd to have the `read` method before setters. But it makes sense to have them sorted :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org