-
Notifications
You must be signed in to change notification settings - Fork 102
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.
Thanks for your contribution aron! Just a couple of points below, keep it up 💪
Codecov Report
@@ Coverage Diff @@
## master #809 +/- ##
==========================================
- Coverage 83.80% 83.77% -0.03%
==========================================
Files 110 111 +1
Lines 3797 3840 +43
==========================================
+ Hits 3182 3217 +35
- Misses 440 444 +4
- Partials 175 179 +4
Continue to review full report at Codecov.
|
pkg/cmd/add_workspace.go
Outdated
workspaceName, _ := cmd.Flags().GetString(workspaceNameFlag) | ||
workspacePath, _ := cmd.Flags().GetString(workspacePathFlag) | ||
|
||
if len(workspaceName) == 0 && len(workspacePath) == 0 { | ||
return formula.Workspace{}, nil, false | ||
} |
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.
what happens if you do rit add workspace --name= --path=
?
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.
It was the same thing when you don't pass any flag.
But, I understood why you'd ask, so I changed to check the number of flags passed before I decided what to do.
wantErr string | ||
}{ | ||
{ | ||
name: "add new workspace by prompt", |
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.
One detail, on a success case, you want to check if the workspace entry was actually added to ~/.rit/formula_workspaces.json
so maybe you do not want to mock the workspace added
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.
I'm not sure if I wanted to check that.
My unit it's until I called the method a.workspace.Add(wspace)
after that the behavior ins't responsability of this method. Because of this I mocked my test.
What do you think?!
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.
I do agree with you, lately we have been trying to adopt a more functional-like approach on our tests. But your argument is good as well, if you intend to continue with the mock, then maybe it is best to test that it has been called with the right arguments to ensure a thorough test. Check out assertCalled
https://godoc.org/github.com/stretchr/testify/mock#Mock.AssertCalled
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.
That's right, I can mock my test, but at least a need to guarantee I'm passing the right parameters.
Thanks for the observation! 😄
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.
Just one last suggestion on the mock but I am good to go. Just not approving now because any further commit would dismiss my review
} | ||
|
||
workspaceMock := &mocks.WorkspaceMock{} | ||
workspaceMock.On("Add", wspace).Return(tt.addWorkspaceWithError) |
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.
nice, this is even better than what I suggested 😄 will be recommending this to the team
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.
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.
Good job 🚀
You just needed to add this command to the core command list, you can add it in the api.go file. we use this to generate our autocomplete of the core commands.
pkg/cmd/add_workspace.go
Outdated
|
||
type addWorkspaceCmd struct { | ||
workspace formula.WorkspaceAddLister | ||
inText prompt.InputText |
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.
could you rename this to input
to make it more readable?
pkg/cmd/add_workspace.go
Outdated
|
||
func (a addWorkspaceCmd) runFormula() CommandRunnerFunc { | ||
return func(cmd *cobra.Command, args []string) error { | ||
if count := cmd.Flags().NFlag(); count == 0 { |
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.
could you simplify it to cmd.Flags().NFlag() == 0
?
Signed-off-by: Aron Richter <aronrichter@gmail.com>
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.
LGTM 🥇
/merge qa |
🔥 Merge Conflict |
/merge qa |
👌 Merged branch feature/add-workspace into qa |
Description
As requested on #802, is necessary a command do add a new workspace.
How to verify it
You can run the command
rit add workspace
and after you can runrit list workspace
and then you can see your workspace addedChangelog
Added a new core command
rit add workspace