add unit tests for cmd/run#3413
Merged
estroz merged 1 commit intooperator-framework:masterfrom Jul 17, 2020
Merged
Conversation
estroz
reviewed
Jul 16, 2020
Comment on lines
+58
to
+77
| if len(args) > 0 { | ||
| if len(args) > 1 { | ||
| return fmt.Errorf("exactly one argument is required") | ||
| } | ||
| c.ManifestsDir = args[0] | ||
| } else { | ||
| c.ManifestsDir = "packagemanifests" | ||
| } |
Member
There was a problem hiding this comment.
This should only be validating a packagemanifestsCmd, not setting fields
Suggested change
| if len(args) > 0 { | |
| if len(args) > 1 { | |
| return fmt.Errorf("exactly one argument is required") | |
| } | |
| c.ManifestsDir = args[0] | |
| } else { | |
| c.ManifestsDir = "packagemanifests" | |
| } | |
| if len(args) != 0 && len(args) > 1 { | |
| return fmt.Errorf("exactly one argument is required") | |
| } |
Set defaults in a function like
func (c *packagemanifestsCmd) setDefaults(args []string) {
if len(args) != 0 {
c.ManifestsDir = args[0]
} else {
c.ManifestsDir = "packagemanifests"
}
}
Contributor
Author
There was a problem hiding this comment.
Yeah, I was trying to keep it simple by have all the pre-parsing stuff happen in validate(), I can break it out into a separate method.
Contributor
Author
There was a problem hiding this comment.
also, I might have to change it in some of the other parts, I think I stuck parsing into validate in some of the other refactors I did.
Contributor
Author
|
changed it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the change:
Add unit tests for the stuff that's staying in cmd/run. Includes a minor refactor to put the arg parsing into a validate method so it's testable, but omitting a separate PR as no functionality changed. Hope that's OK?
Also added a dir with a dummy project file to test the behavior where it's supposed to automatically set the manifest dir if a project file is present. I opted to put a real file on disk and include it in the repo as opposed to writing something temp from the code. Let me know if the other way would be preferable.
Motivation for the change:
As part of ongoing work for #3246