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.

Sign up for GitHub

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

[Fix] Code Refactor & Cleanup #154

Closed
wants to merge 2 commits into from
+64 −49
Closed

[Fix] Code Refactor & Cleanup #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e892a8
(fix) : lint and typo
VILJkid Jun 21, 2024
0642856
(fix) : code refactor and cleanup
VILJkid Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions all_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func TestMain(m *testing.M) {
os.Exit(code)
}

func teardown(m *testing.M) {
func teardown(_ *testing.M) {
os.RemoveAll("test/data/case03/case01")
os.Remove("test/data/case03/relative_foo")
os.RemoveAll("test/data.copy")
Expand Down Expand Up @@ -420,7 +420,7 @@ func TestOptions_FS(t *testing.T) {
os.RemoveAll("test/data/case18/assets")
err := Copy("test/data/case18/assets", "test/data.copy/case18/assets", Options{
FS: assets,
PermissionControl: AddPermission(200), // FIXME
PermissionControl: AddPermission(0200), // FIXME
})
Expect(t, err).ToBe(nil)
}
Expand Down Expand Up @@ -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") {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this.

return strings.TrimSuffix(d, ".tpl"), nil
}
return d, nil // otherwise do nothing
Expand Down
95 changes: 54 additions & 41 deletions copy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,18 +86,12 @@ func copyNextOrSkip(src, dest string, info os.FileInfo, opt Options) error {
// with considering existence of parent directory
// and file permission.
func fcopy(src, dest string, info os.FileInfo, opt Options) (err error) {

var readcloser io.ReadCloser
if opt.FS != nil {
readcloser, err = opt.FS.Open(src)
} else {
readcloser, err = os.Open(src)
}
readcloser, err := fopen(src, opt)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the following function dread, you give only opt.FS. What's the difference between this and that?

if err != nil {
if os.IsNotExist(err) {
return nil
if !os.IsNotExist(err) {
return
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

}
return
return nil
}
defer fclose(readcloser, &err)

Expand Down Expand Up @@ -155,14 +149,23 @@ func fcopy(src, dest string, info os.FileInfo, opt Options) (err error) {
return
}

// fopen opens the named file,
// and returns it regardless of its type
// fs.File or *os.File
func fopen(src string, opt Options) (io.ReadCloser, error) {
if opt.FS != nil {
return opt.FS.Open(src)
}
return os.Open(src)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this.


// dcopy is for a directory,
// with scanning contents inside the directory
// and pass everything to "copy" recursively.
func dcopy(srcdir, destdir string, info os.FileInfo, opt Options) (err error) {
if skip, err := onDirExists(opt, srcdir, destdir); err != nil {
return err
} else if skip {
return nil
skip, err := onDirExists(opt, srcdir, destdir)
if err != nil || skip {
return
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?


// Make dest dir with 0755 so that everything writable.
Expand All @@ -172,20 +175,9 @@ func dcopy(srcdir, destdir string, info os.FileInfo, opt Options) (err error) {
}
defer chmodfunc(&err)

var entries []fs.DirEntry
if opt.FS != nil {
entries, err = fs.ReadDir(opt.FS, srcdir)
if err != nil {
return err
}
} else {
entries, err = os.ReadDir(srcdir)
if err != nil {
if os.IsNotExist(err) {
return nil
}
return err
}
entries, err := dread(srcdir, opt.FS)
if err != nil || entries == nil {
return
}

contents := make([]fs.FileInfo, 0, len(entries))
Expand All @@ -197,16 +189,12 @@ func dcopy(srcdir, destdir string, info os.FileInfo, opt Options) (err error) {
contents = append(contents, info)
}

if yes, err := shouldCopyDirectoryConcurrent(opt, srcdir, destdir); err != nil {
return err
} else if yes {
if err := dcopyConcurrent(srcdir, destdir, contents, opt); err != nil {
return err
}
} else {
if err := dcopySequential(srcdir, destdir, contents, opt); err != nil {
return err
}
shouldCopyConcurrent, err := shouldCopyDirectoryConcurrent(opt, srcdir, destdir)
if err != nil {
return
}
if err = chooseAndPerformCopyMethod(shouldCopyConcurrent, srcdir, destdir, contents, opt); err != nil {
return
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any benefit of separating this block to a function.

}

if opt.PreserveTimes {
Expand All @@ -224,6 +212,30 @@ func dcopy(srcdir, destdir string, info os.FileInfo, opt Options) (err error) {
return
}

// dread reads the named directory,
// it regardless if it's a filesystem
// and returns list of directory entries.
func dread(srcdir string, fsys fs.FS) ([]fs.DirEntry, error) {
if fsys != nil {
return fs.ReadDir(fsys, srcdir)
}
entries, err := os.ReadDir(srcdir)
if err != nil {
if !os.IsNotExist(err) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even in fs.FS, let's ignore os.IsNotExist, then this function can be more readable.

return nil, err
}
return nil, nil
}
return entries, nil
}

func chooseAndPerformCopyMethod(shouldCopyConcurrent bool, srcdir, destdir string, contents []fs.FileInfo, opt Options) error {
if shouldCopyConcurrent {
return dcopyConcurrent(srcdir, destdir, contents, opt)
}
return dcopySequential(srcdir, destdir, contents, opt)
}

func dcopySequential(srcdir, destdir string, contents []os.FileInfo, opt Options) error {
for _, content := range contents {
cs, cd := filepath.Join(srcdir, content.Name()), filepath.Join(destdir, content.Name())
Expand Down Expand Up @@ -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) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this condition separation and moving exception case first.

return true, err // Unwelcome error type...!
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this comment.

}
if opt.OnDirExists != nil && destdir != opt.intent.dest {
switch opt.OnDirExists(srcdir, destdir) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, we can separate another exception case destdir == opt.intent.dest on the top of this function for early-return.

case Replace:
if err := os.RemoveAll(destdir); err != nil {
Expand All @@ -271,8 +286,6 @@ func onDirExists(opt Options, srcdir, destdir string) (bool, error) {
case Untouchable:
return true, nil
} // case "Merge" is default behaviour. Go through.
} else if err != nil && !os.IsNotExist(err) {
return true, err // Unwelcome error type...!
}
return false, nil
}
Expand Down
7 changes: 4 additions & 3 deletions options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to keep backward compatibility, we can still keep PerservePermission with // Deprecated: marker.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accordingly, let's painfully respect PerservePermission but prioritize PreservePermission in other code blocks.

Sync: false, // Do not sync
Specials: false, // Do not copy special files
PreserveTimes: false, // Do not preserve the modification time
Expand All @@ -155,8 +155,9 @@ func assureOptions(src, dest string, opts ...Options) Options {
}
if opts[0].AddPermission > 0 {
opts[0].PermissionControl = AddPermission(opts[0].AddPermission)
} else if opts[0].PermissionControl == nil {
opts[0].PermissionControl = PerservePermission
}
if opts[0].PermissionControl == nil {
opts[0].PermissionControl = PreservePermission
}
opts[0].intent.src = defopt.intent.src
opts[0].intent.dest = defopt.intent.dest
Expand Down
2 changes: 1 addition & 1 deletion permission_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ var (
}, nil
}
}
PerservePermission PermissionControlFunc = AddPermission(0)
PreservePermission PermissionControlFunc = AddPermission(0)
DoNothing PermissionControlFunc = func(srcinfo fs.FileInfo, dest string) (func(*error), error) {
if srcinfo.IsDir() {
if err := os.MkdirAll(dest, srcinfo.Mode()); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion test_setup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"testing"
)

func setup(m *testing.M) {
func setup(_ *testing.M) {
os.RemoveAll("test/data.copy")
os.MkdirAll("test/data.copy", os.ModePerm)
os.Symlink("../case01", "test/data/case03/case01")
Expand Down
Loading