[Fix] Code Refactor & Cleanup #154
Conversation
VILJkid
changed the title
| if os.IsNotExist(err) { | ||
| return nil | ||
| if !os.IsNotExist(err) { | ||
| return |
There was a problem hiding this comment.
[Just IMO for readability] 🤔
In this case, if os.IsNotExist(err) is the exceptional case.
Code-readers expect if err != nil, then return err as usual. But the interesting point is that we do not return the error if it's ErrNotExist.
To represent this flow of thinking, I do prefer using if os.IsNotExist(err); then return nil.
In addition, !os.IsNotExist(err) is a double-negative way.
| @@ -461,7 +461,8 @@ func TestOptions_RenameDestination(t *testing.T) { | |||
| // Your own logic here | |||
| if strings.HasSuffix(s, ".template") { | |||
| return strings.TrimSuffix(d, ".template"), nil | |||
| } else if strings.HasSuffix(s, ".tpl") { | |||
| } | |||
| if strings.HasSuffix(s, ".tpl") { | |||
| return opt.FS.Open(src) | ||
| } | ||
| return os.Open(src) | ||
| } |
| return nil | ||
| skip, err := onDirExists(opt, srcdir, destdir) | ||
| if err != nil || skip { | ||
| return | ||
| } |
There was a problem hiding this comment.
[Just IMO for readability] 🤔
Returning error on error and skip this dcopy if on skip == true are different things. Thus I prefer separating those 2 in two returns.
What do you think?
| } else { | ||
| readcloser, err = os.Open(src) | ||
| } | ||
| readcloser, err := fopen(src, opt) |
There was a problem hiding this comment.
In the following function dread, you give only opt.FS. What's the difference between this and that?
| return | ||
| } | ||
| if err = chooseAndPerformCopyMethod(shouldCopyConcurrent, srcdir, destdir, contents, opt); err != nil { | ||
| return |
There was a problem hiding this comment.
I don't see any benefit of separating this block to a function.
| @@ -262,7 +274,10 @@ func dcopyConcurrent(srcdir, destdir string, contents []os.FileInfo, opt Options | |||
|
|
|||
| func onDirExists(opt Options, srcdir, destdir string) (bool, error) { | |||
| _, err := os.Stat(destdir) | |||
| if err == nil && opt.OnDirExists != nil && destdir != opt.intent.dest { | |||
| if err != nil && !os.IsNotExist(err) { | |||
| return true, err // Unwelcome error type...! | |||
| @@ -262,7 +274,10 @@ func dcopyConcurrent(srcdir, destdir string, contents []os.FileInfo, opt Options | |||
|
|
|||
| func onDirExists(opt Options, srcdir, destdir string) (bool, error) { | |||
| _, err := os.Stat(destdir) | |||
| if err == nil && opt.OnDirExists != nil && destdir != opt.intent.dest { | |||
| if err != nil && !os.IsNotExist(err) { | |||
There was a problem hiding this comment.
I love this condition separation and moving exception case first.
| if err != nil && !os.IsNotExist(err) { | ||
| return true, err // Unwelcome error type...! | ||
| } | ||
| if opt.OnDirExists != nil && destdir != opt.intent.dest { | ||
| switch opt.OnDirExists(srcdir, destdir) { |
There was a problem hiding this comment.
IMO, we can separate another exception case destdir == opt.intent.dest on the top of this function for early-return.
| @@ -130,7 +130,7 @@ func getDefaultOptions(src, dest string) Options { | |||
| OnError: nil, // Default is "accept error" | |||
| Skip: nil, // Do not skip anything | |||
| AddPermission: 0, // Add nothing | |||
| PermissionControl: PerservePermission, // Just preserve permission | |||
| PermissionControl: PreservePermission, // Just preserve permission | |||
There was a problem hiding this comment.
In order to keep backward compatibility, we can still keep PerservePermission with // Deprecated: marker.
There was a problem hiding this comment.
Accordingly, let's painfully respect PerservePermission but prioritize PreservePermission in other code blocks.
Issue #155
Changes introduced
_the unused variables in functionsPerservePermissiontoPreservePermissiontypo fixreturnselseconditions