-
Notifications
You must be signed in to change notification settings - Fork 268
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
Example for interacting with remote file servers #1061
Conversation
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.
The tests are failing, please check. Also while fixing please update the branch as well.
@RahulMohanK please resolve the review comments given by @srijan-27. Thankyou |
…files-interaction
@srijan-27 @Umang01-hash Resolved the issue with the unit tests. |
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.
@RahulMohanK the PR looks good but i would suggest to add some spacing in the file main_test.go
to improve the readability of code.
(Variable declaratrion, catching logs and assertions, you can separate them using proper spacing).
Also i saw the we have used var logger logging.Logger
as a global variable in the file main.go
. We should be avoiding this and instead pass logger in the function/method wherever required.
@Umang01-hash Refactored test file to increase the readability of the code. Passing logger into function instead of declaring a global variable. |
…files-interaction
@RahulMohanK there are a few review comments left by @aryanmehrotra. Please address them |
…files-interaction
@RahulMohanK could you please resolve the review comments left by @aryanmehrotra and @shashank-zopsmart? I think they raised some key code and quality issues with the PR. |
if err != nil { | ||
fmt.Println(err) | ||
} else { | ||
printFiles(files) | ||
} | ||
|
||
return "", err |
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.
if err != nil { | |
fmt.Println(err) | |
} else { | |
printFiles(files) | |
} | |
return "", err | |
if err != nil { | |
return fmt.Sprintf("failed to read directory: %s", path), err | |
} | |
printFiles(files) | |
return "", nil |
if err != nil { | ||
fmt.Println(err) | ||
} else { | ||
grepFiles(files, keyword) | ||
|
||
} | ||
|
||
return "", err |
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.
Similar to previous comment
if err == nil { | ||
return fmt.Sprintln("Successfully created file:", fileName), nil | ||
} | ||
|
||
return fmt.Sprintln("File Creation error"), err |
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.
if err == nil { | |
return fmt.Sprintln("Successfully created file:", fileName), nil | |
} | |
return fmt.Sprintln("File Creation error"), err | |
if err != nil { | |
return fmt.Sprintln("File Creation error"), err | |
} | |
return fmt.Sprintln("Successfully created file:", fileName), nil |
if err == nil { | ||
return fmt.Sprintln("Successfully removed file:", fileName), nil | ||
} | ||
|
||
return fmt.Sprintln("File removal error"), err |
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.
Similar to previous comment
@RahulMohanK can you address the review changes |
The work of this PR has been addressed in PR #1177. Thankyou for your valuable contribution @RahulMohanK. |
Pull Request Template
Description:
Additional Information:
Checklist:
goimport
andgolangci-lint
.Thank you for your contribution!