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.

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

(refactor) cmd refactor #22

Merged
merged 6 commits into from
Jun 29, 2024
Merged

(refactor) cmd refactor #22

merged 6 commits into from
Jun 29, 2024

Conversation

kevincobain2000
Copy link
Owner

@kevincobain2000 kevincobain2000 commented Jun 29, 2024

Refactoring first as part of #1

Copy link

CoverItUp Report

Type master feature/refactor c3a6cf7 from da19f3b
npm-install-time 6sec 6sec
npm-build-time 7sec 6sec 📉
go-build-time 8sec 8sec
go-lint-errors 0 0
go-test-run-time 21 20 📉
coverage 23.2% 23.2%
go-binary-size 9.0kKB 9.0kKB 📈
go-mod-dependencies 203 203
Comparisons Chart - master from feature/refactor

base vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branch

Commits History

Upto da19f3b for #22
commit historycommit historycommit historycommit historycommit historycommit historycommit historycommit history

Users History

Upto da19f3b for #22
user historyuser historyuser historyuser historyuser historyuser historyuser historyuser history

Embed README.md

Copy link

CoverItUp Report

Type master feature/refactor c3a6cf7 from da19f3b
coverage 23.2% 23.2%
go-mod-dependencies 203 203
Comparisons Chart - master from feature/refactor

base vs branchbase vs branch

Commits History

Upto da19f3b for #22
commit historycommit history

Users History

Upto da19f3b for #22
user historyuser history

Embed README.md

pkg/slices.go Show resolved Hide resolved
pkg/system.go Outdated Show resolved Hide resolved
pkg/slices.go Show resolved Hide resolved
main.go Outdated
}

func wantsVersion() {
if f.version {
if len(os.Args) == 2 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

func wantsVersion() { 
-         if len(os.Args) == 2 && 
-                 (os.Args[1] == "-version" || os.Args[1] == "--version" || 
-                         os.Args[1] == "-v" || os.Args[1] == "--v" || 
-                         os.Args[1] == "version") || f.version { 
-                  fmt.Println(version) 
-                 os.Exit(0) 
-          }
+        if  len(os.Args) != 2 {
+                  return
+         }
+         switch os.Args[1] {
+               case "-v", "--v", "-version", "--version":
+            fmt.Println(version) 
+                os.Exit(0) 
+         }
}

pkg/files.go Show resolved Hide resolved
@kevincobain2000
Copy link
Owner Author

Thanks @ccoVeille
Will commit suggestions and fix based on comments.

@ccoVeille
Copy link
Contributor

I'm glad you find my review useful

}

// provide a Set() method on the type so we can use it with flag.Var
func (i *SliceFlags) Set(value string) error {
Copy link
Owner Author

@kevincobain2000 kevincobain2000 Jun 29, 2024

Choose a reason for hiding this comment

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

This can't be append, reverting back from append -> set

Reason:

./main.go:201:11: cannot use &f.filePaths (value of type *"github.com/kevincobain2000/gol/pkg".SliceFlags) as flag.Value value in argument to flag.Var: *"github.com/kevincobain2000/gol/pkg".SliceFlags does not implement flag.Value (missing method Set)

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, in such case, I always try to add a comment like:

// required to implement flag.Value interface

Copy link

CoverItUp Report

Comparison Table - 8 Types 📈
Type master feature/refactor c3a6cf7 from f708113
npm-install-time 6sec 6sec
npm-build-time 7sec 6sec 📉
go-build-time 8sec 10sec 📈
go-lint-errors 0 0
go-test-run-time 21 21
coverage 23.2% 23.2%
go-binary-size 9.0kKB 9.0kKB 📈
go-mod-dependencies 203 203
Comparisons Chart - master from feature/refactor

base vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branch

Commits History

Upto f708113 for #22
commit historycommit historycommit historycommit historycommit historycommit historycommit historycommit history

Users History

Upto f708113 for #22
user historyuser historyuser historyuser historyuser historyuser historyuser historyuser history

Embed README.md

Copy link

CoverItUp Report

Comparison Table - 2 Types
Type master feature/refactor c3a6cf7 from f708113
coverage 23.2% 23.2%
go-mod-dependencies 203 203
Comparisons Chart - master from feature/refactor

base vs branchbase vs branch

Commits History

Upto f708113 for #22
commit historycommit history

Users History

Upto f708113 for #22
user historyuser history

Embed README.md

Copy link

CoverItUp Report

Comparison Table - 8 Types 📈
Type master feature/refactor c3a6cf7 from cab890b
npm-install-time 6sec 6sec
npm-build-time 7sec 6sec 📉
go-build-time 8sec 9sec 📈
go-lint-errors 0 0
go-test-run-time 21 21
coverage 23.2% 23.7% 📈
go-binary-size 9.0kKB 9.0kKB 📈
go-mod-dependencies 203 203
Comparisons Chart - master from feature/refactor

base vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branch

Commits History

Upto cab890b for #22
commit historycommit historycommit historycommit historycommit historycommit historycommit historycommit history

Users History

Upto cab890b for #22
user historyuser historyuser historyuser historyuser historyuser historyuser historyuser history

Embed README.md

Copy link

CoverItUp Report

Comparison Table - 2 Types 📈
Type master feature/refactor c3a6cf7 from cab890b
coverage 23.2% 23.7% 📈
go-mod-dependencies 203 203
Comparisons Chart - master from feature/refactor

base vs branchbase vs branch

Commits History

Upto cab890b for #22
commit historycommit history

Users History

Upto cab890b for #22
user historyuser history

Embed README.md

Copy link

CoverItUp Report

Comparison Table - 8 Types 📈
Type master feature/refactor c3a6cf7 from 0e24206
npm-install-time 6sec 11sec 📈
npm-build-time 7sec 7sec
go-build-time 8sec 9sec 📈
go-lint-errors 0 0
go-test-run-time 21 21
coverage 23.2% 23.6% 📈
go-binary-size 9.0kKB 9.0kKB 📈
go-mod-dependencies 203 203
Comparisons Chart - master from feature/refactor

base vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branchbase vs branch

Commits History

Upto 0e24206 for #22
commit historycommit historycommit historycommit historycommit historycommit historycommit historycommit history

Users History

Upto 0e24206 for #22
user historyuser historyuser historyuser historyuser historyuser historyuser historyuser history

Embed README.md

Copy link

CoverItUp Report

Comparison Table - 2 Types 📈
Type master feature/refactor c3a6cf7 from 0e24206
coverage 23.2% 23.6% 📈
go-mod-dependencies 203 203
Comparisons Chart - master from feature/refactor

base vs branchbase vs branch

Commits History

Upto 0e24206 for #22
commit historycommit history

Users History

Upto 0e24206 for #22
user historyuser history

Embed README.md

@kevincobain2000 kevincobain2000 merged commit ea98a52 into master Jun 29, 2024
2 checks passed
@kevincobain2000 kevincobain2000 deleted the feature/refactor branch June 29, 2024 20:36
pkg/files.go Outdated
@@ -383,3 +383,10 @@ func UniqueFileInfos(fileInfos []FileInfo) []FileInfo {
}
return list
}

// func UniqueFileInfos(fileInfos []FileInfo) []FileInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

You kept them here for a later use ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Removed them. I tested the asis and Tobe first, as there were no tests for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants