Extract download type from download command #821
Conversation
| payload *downloadPayload | ||
| } | ||
|
|
||
| func newDownload(flags *pflag.FlagSet, usrCfg *viper.Viper) (*download, error) { |
There was a problem hiding this comment.
This is currently longer than ideal. I think it makes sense to have the logic to populate the payload in the constructor though. Clients can't do anything with a download without the payload being populated and extracting it to a method doesn't seem appropriate because clients should never need to call it - it just needs to happen on creation.
There was a problem hiding this comment.
Yeah, it would either have to be populated in the constructor, or have a separate method that always gets called before we need those values. The second seems like an unnecessary abstraction.
| slug, uuid string | ||
|
|
||
| // user config | ||
| token, apibaseurl, workspace string |
There was a problem hiding this comment.
I'm not sure if most would agree to save these an individual fields rather than the config object. Calling GetString throughout the system seems vulnerable to typo's where using fields has the benefit of the compiler catching it.
There was a problem hiding this comment.
I actually really like having them here like that. The flags feel like they should be processed once, and that's it.
| } | ||
|
|
||
| // needsUserConfigValues checks the presence of required values from the user config. | ||
| func (d download) needsUserConfigValues() error { |
There was a problem hiding this comment.
In cmd/cmd we have func validateUserConfig(cfg *viper.Viper) but coupling to an outside validation seems like a bad idea so that's why it's duplicated here.
jdsutherland
force-pushed
the
refactor-download
branch
from
b40b413 to
d5ffa73
Compare
Unneeded as these checks already happen in the constructor and it's potentially confusing because there are other needsX validation methods
|
Sweet! I'll look at this today. This looks like a very manageable size to review, I appreciate that very much 💙 💚 💛 |
| slug, uuid string | ||
|
|
||
| // user config | ||
| token, apibaseurl, workspace string |
There was a problem hiding this comment.
I actually really like having them here like that. The flags feel like they should be processed once, and that's it.
| payload *downloadPayload | ||
| } | ||
|
|
||
| func newDownload(flags *pflag.FlagSet, usrCfg *viper.Viper) (*download, error) { |
There was a problem hiding this comment.
Yeah, it would either have to be populated in the constructor, or have a separate method that always gets called before we need those values. The second seems like an unnecessary abstraction.
| } | ||
| defer res.Body.Close() | ||
|
|
||
| if err := json.NewDecoder(res.Body).Decode(&d.payload); err != nil { |
This is an attempt at a minimal first change in refactoring the download command. This change adds a download type.