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

PR feedback and comments #88

Merged
merged 1 commit into from
May 9, 2023
Merged

PR feedback and comments #88

merged 1 commit into from
May 9, 2023

Conversation

radsec
Copy link
Collaborator

@radsec radsec commented May 9, 2023

TO: @discentem @natewalck

CHANGES

CONTEXT

Because I am still learning Sapling...I didn't perform the stacked-PR correctly so this PR contains all of the PR feedback from the last one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I forgot this file the first time but this tests all the required flags and required args for the initCmd

@@ -59,7 +63,7 @@ func initRunE(cmd *cobra.Command, args []string) error {

switch stores.StoreType(backend) {
case stores.StoreTypeS3:
cfg = config.InitializeStoreTypeS3Config(
cfg = config.InitializeStoreTypeS3(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comment to simplify the InitStoreTypeS3 fn name

@@ -10,14 +10,14 @@ import (
"github.com/spf13/viper"
)

func initCommand() *cobra.Command {
func initCmd() *cobra.Command {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

standardized the fn Cmd() names as recommended

@@ -8,15 +8,15 @@ import (
"github.com/stretchr/testify/require"
)

func TestRootCommand(t *testing.T) {
func TestRootCmd(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

aligned with golang standards on unit testing fn names

}

return uploadCmd
}

func uploadRunE(cmd *cobra.Command, objects []string) error {
// uploadFn is the execution runtime for the uploadCmd functionality
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the start for godoc generation to document functions inline with comments

initCommand(),
retrieveCommand(),
uploadCommand(),
initCmd(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

align with fn Cmd() names

subArgs,
)

// Check the required flags exist:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

test that all of the required flags appear...this is important as it ensures we don't miss this in the implementation and aligns our docs with cmd functionality

@@ -27,6 +27,7 @@ go_test(
name = "cli_test",
srcs = [
"helpers_test.go",
"init_test.go",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moar tests!!

@radsec radsec requested a review from natewalck May 9, 2023 20:47
internal/cli/init.go Outdated Show resolved Hide resolved
@radsec radsec merged commit 4bfeca1 into main May 9, 2023
@radsec radsec deleted the pr88 branch May 9, 2023 22:57
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