isolate repo clone #1416
isolate repo clone #1416
Conversation
This commit hacks the existing repo clone tests into something usable by the new isolated command. It went ok and was less effort than trying to introduce the same kind of test format as repo view and gist create.
| return c.errBuf.String() | ||
| } | ||
|
|
||
| func runCloneCommand(httpClient *http.Client, cli string) (*cmdOut, error) { |
There was a problem hiding this comment.
My thinking is that each new isolated command will come with a run*Command function that replaces use of runCommand in tests. This worked well here.
There was a problem hiding this comment.
Great approach! I would argue that we could even name it RunCommand scoped to the new package so we don't have to rename any existing RunCommand calls from tests that we copy over 🤘
| return baseRepo, nil | ||
| } | ||
|
|
||
| repoResolvingCmdFactory := *cmdFactory |
There was a problem hiding this comment.
Oooh is this a way to create a shallow copy of a struct? Seems really nifty
|
|
||
| repoResolvingCmdFactory := *cmdFactory | ||
|
|
||
| repoResolvingCmdFactory.BaseRepo = resolvedBaseRepo |
There was a problem hiding this comment.
I wonder if we could/should make the result of BaseRepo cached? Just in case a command calls it more than once (it usually doesn't); right now that would perform full resolution (w/ API call) every time.
There was a problem hiding this comment.
I'm fine with this being uncached for a few reasons:
- the caller will probably most often be saving the result in a variable anyway
- we don't currently have any commands that need to call this more than once
- we might move away from "smart" resolution, anyway
| @@ -94,6 +96,15 @@ func init() { | |||
| ctx := context.New() | |||
| return ctx.BaseRepo() | |||
| }, | |||
| Config: func() (config.Config, error) { | |||
There was a problem hiding this comment.
The addition of Config here is great. Same for BaseRepo, should we perhaps cache the result after the 1st call? In case a command consults a config multiple times (e.g. looking up different config options), we'd avoid re-parsing the configuration file from disk every time. (Not that it's anywhere near as expensive as an API call)
There was a problem hiding this comment.
I'm more amenable to caching this than BaseRepo but also think it's something we can put off to keep the code simpler as we continue with this migration.
| return c.errBuf.String() | ||
| } | ||
|
|
||
| func runCloneCommand(httpClient *http.Client, cli string) (*cmdOut, error) { |
There was a problem hiding this comment.
Great approach! I would argue that we could even name it RunCommand scoped to the new package so we don't have to rename any existing RunCommand calls from tests that we copy over 🤘
Depends on #1409
Part of #1405
Part of #1082
This PR isolates
gh repo clone.Unlike the previous isolation PRs, this one reuses existing tests. This did require some massaging,
but was a lot faster and easier then re-writing the test cases from scratch.
I moved some stuff around to share code between non-isolated and isolated commands; we can split
stuff back out of
gitas needed once everything is isolated.I did add a new field to
cmdutil.Factory:Config(). This seemed like the correct thing but I'mcurious what your thoughts are, Mislav.