-
Notifications
You must be signed in to change notification settings - Fork 602
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
adds fallback to [email protected] extension if possible and c… #827
Conversation
…ommunicates possible problems with flags to the developer
|
// build and send request | ||
final Request request = newRequest(type); | ||
|
||
if (serverExtension != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add {
around the if-statement... Everywhere where that's not done is a leftover ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, i just pushed a corrected version.
Thanks for your quick reply.
Maybe only thing missing is some (unit) tests |
Codecov Report
@@ Coverage Diff @@
## master #827 +/- ##
============================================
+ Coverage 68.40% 68.55% +0.14%
- Complexity 1398 1412 +14
============================================
Files 207 207
Lines 7478 7504 +26
Branches 631 642 +11
============================================
+ Hits 5115 5144 +29
+ Misses 2025 2015 -10
- Partials 338 345 +7
|
I added some simple tests to cover sftp.rename(). |
The integration tests run against an OpenSSH server, so that should be testable. |
Sorry, I don't have much time at the moment. |
The integration tests have been simplified significantly I think. It should not be hard to add a few tests to cover this behaviour now. |
I tried to make the tests as future-proof as possible by allowing to request a specific SFTP protocol version. This way things will not break/become non-functional when sshj will support SFTPv5 or greater in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small nitpick to prevent too many externally visible methods, otherwise great work!
Merged, thanks for the PR! |
You're welcome. Thanks for your patience! |
adds fallback to [email protected] extension if possible and communicates possible problems with flags to the developer
Tested with openssh-sftp-server-1:8.9p1-3 (Ubuntu 22.04)