-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Extract downloadWriter from download command #826
Changes from all commits
f2db586
a469bf5
5f42f09
39a8b36
f26ca62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ import ( | |
|
||
"github.com/exercism/cli/api" | ||
"github.com/exercism/cli/config" | ||
"github.com/exercism/cli/workspace" | ||
ws "github.com/exercism/cli/workspace" | ||
"github.com/spf13/cobra" | ||
"github.com/spf13/pflag" | ||
"github.com/spf13/viper" | ||
|
@@ -59,83 +59,16 @@ func runDownload(cfg config.Config, flags *pflag.FlagSet, args []string) error { | |
return err | ||
} | ||
|
||
metadata := download.payload.metadata() | ||
dir := metadata.Exercise(usrCfg.GetString("workspace")).MetadataDir() | ||
|
||
if err := os.MkdirAll(dir, os.FileMode(0755)); err != nil { | ||
return err | ||
} | ||
|
||
if err := metadata.Write(dir); err != nil { | ||
writer := newDownloadWriter(download) | ||
if err := writer.writeSolutionFiles(); err != nil { | ||
return err | ||
} | ||
|
||
client, err := api.NewClient(usrCfg.GetString("token"), usrCfg.GetString("apibaseurl")) | ||
if err != nil { | ||
if err := writer.writeMetadata(); err != nil { | ||
return err | ||
} | ||
|
||
for _, file := range download.payload.Solution.Files { | ||
unparsedURL := fmt.Sprintf("%s%s", download.payload.Solution.FileDownloadBaseURL, file) | ||
parsedURL, err := netURL.ParseRequestURI(unparsedURL) | ||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
url := parsedURL.String() | ||
|
||
req, err := client.NewRequest("GET", url, nil) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
res, err := client.Do(req) | ||
if err != nil { | ||
return err | ||
} | ||
defer res.Body.Close() | ||
|
||
if res.StatusCode != http.StatusOK { | ||
// TODO: deal with it | ||
continue | ||
} | ||
// Don't bother with empty files. | ||
if res.Header.Get("Content-Length") == "0" { | ||
continue | ||
} | ||
|
||
// TODO: if there's a collision, interactively resolve (show diff, ask if overwrite). | ||
// TODO: handle --force flag to overwrite without asking. | ||
|
||
// Work around a path bug due to an early design decision (later reversed) to | ||
// allow numeric suffixes for exercise directories, allowing people to have | ||
// multiple parallel versions of an exercise. | ||
pattern := fmt.Sprintf(`\A.*[/\\]%s-\d*/`, metadata.ExerciseSlug) | ||
rgxNumericSuffix := regexp.MustCompile(pattern) | ||
if rgxNumericSuffix.MatchString(file) { | ||
file = string(rgxNumericSuffix.ReplaceAll([]byte(file), []byte(""))) | ||
} | ||
|
||
// Rewrite paths submitted with an older, buggy client where the Windows path is being treated as part of the filename. | ||
file = strings.Replace(file, "\\", "/", -1) | ||
|
||
relativePath := filepath.FromSlash(file) | ||
dir := filepath.Join(metadata.Dir, filepath.Dir(relativePath)) | ||
os.MkdirAll(dir, os.FileMode(0755)) | ||
|
||
f, err := os.Create(filepath.Join(metadata.Dir, relativePath)) | ||
if err != nil { | ||
return err | ||
} | ||
defer f.Close() | ||
_, err = io.Copy(f, res.Body) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
fmt.Fprintf(Err, "\nDownloaded to\n") | ||
fmt.Fprintf(Out, "%s\n", metadata.Dir) | ||
fmt.Fprintf(Out, "%s\n", writer.destination()) | ||
return nil | ||
} | ||
|
||
|
@@ -149,7 +82,7 @@ type download struct { | |
// optional | ||
track, team string | ||
|
||
payload *downloadPayload | ||
*downloadPayload | ||
} | ||
|
||
func newDownload(flags *pflag.FlagSet, usrCfg *viper.Viper) (*download, error) { | ||
|
@@ -203,7 +136,7 @@ func newDownload(flags *pflag.FlagSet, usrCfg *viper.Viper) (*download, error) { | |
} | ||
defer res.Body.Close() | ||
|
||
if err := json.NewDecoder(res.Body).Decode(&d.payload); err != nil { | ||
if err := json.NewDecoder(res.Body).Decode(&d.downloadPayload); err != nil { | ||
return nil, decodedAPIError(res) | ||
} | ||
|
||
|
@@ -264,6 +197,119 @@ func (d download) needsSlugWhenGivenTrackOrTeam() error { | |
return nil | ||
} | ||
|
||
func (d download) metadata() *ws.ExerciseMetadata { | ||
return &ws.ExerciseMetadata{ | ||
AutoApprove: d.Solution.Exercise.AutoApprove, | ||
Track: d.Solution.Exercise.Track.ID, | ||
Team: d.Solution.Team.Slug, | ||
ExerciseSlug: d.Solution.Exercise.ID, | ||
ID: d.Solution.ID, | ||
URL: d.Solution.URL, | ||
Handle: d.Solution.User.Handle, | ||
IsRequester: d.Solution.User.IsRequester, | ||
} | ||
} | ||
|
||
func (d download) exercise() ws.Exercise { | ||
return d.metadata().Exercise(d.workspace) | ||
} | ||
|
||
// requestFile requests a Solution file from the API, returning an HTTP response. | ||
// 0 Content-Length responses are swallowed, returning nil. | ||
func (d download) requestFile(filename string) (*http.Response, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could see this being its own function not tied to download. Maybe this is something that should live on the Exercism Client API with a single argument being the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of having a helper on the client API with a single URL argument. That feels like a perfectly reasonable (and cohesive!) bit of behavior for the CLI. |
||
parsedURL, err := netURL.ParseRequestURI( | ||
fmt.Sprintf("%s%s", d.Solution.FileDownloadBaseURL, filename)) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
client, err := api.NewClient(d.token, d.apibaseurl) | ||
req, err := client.NewRequest("GET", parsedURL.String(), nil) | ||
if err != nil { | ||
return nil, err | ||
} | ||
res, err := client.Do(req) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if res.StatusCode != http.StatusOK { | ||
return nil, decodedAPIError(res) | ||
} | ||
// Don't bother with empty files. | ||
if res.Header.Get("Content-Length") == "0" { | ||
return nil, nil | ||
} | ||
|
||
return res, nil | ||
} | ||
|
||
// downloadWriter writes download contents to the file system. | ||
type downloadWriter struct { | ||
download *download | ||
} | ||
|
||
// newDownloadWriter creates a downloadWriter. | ||
func newDownloadWriter(dl *download) *downloadWriter { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could get a way with just initializing a literal downloadWriter in |
||
return &downloadWriter{download: dl} | ||
} | ||
|
||
// writeMetadata writes the exercise metadata. | ||
func (w downloadWriter) writeMetadata() error { | ||
return w.download.metadata().Write(w.destination()) | ||
} | ||
|
||
// writeSolutionFiles attempts to write each file from the downloaded solution. | ||
func (w downloadWriter) writeSolutionFiles() error { | ||
for _, filename := range w.download.Solution.Files { | ||
res, err := w.download.requestFile(filename) | ||
if err != nil { | ||
return err | ||
} | ||
if res == nil { | ||
// Ignore empty responses | ||
continue | ||
} | ||
defer res.Body.Close() | ||
|
||
destination := filepath.Join( | ||
w.destination(), | ||
w.sanitizeLegacyFilepath(filename, w.download.exercise().Slug)) | ||
if err = os.MkdirAll(filepath.Dir(destination), os.FileMode(0755)); err != nil { | ||
return err | ||
} | ||
f, err := os.Create(destination) | ||
if err != nil { | ||
return err | ||
} | ||
defer f.Close() | ||
if _, err := io.Copy(f, res.Body); err != nil { | ||
return err | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
// destination is the download destination path. | ||
func (w downloadWriter) destination() string { | ||
return w.download.exercise().MetadataDir() | ||
} | ||
|
||
// sanitizeLegacyFilepath is a workaround for a path bug due to an early design | ||
// decision (later reversed) to allow numeric suffixes for exercise directories, | ||
// allowing people to have multiple parallel versions of an exercise. | ||
func (d downloadWriter) sanitizeLegacyFilepath(file, slug string) string { | ||
pattern := fmt.Sprintf(`\A.*[/\\]%s-\d*/`, slug) | ||
rgxNumericSuffix := regexp.MustCompile(pattern) | ||
if rgxNumericSuffix.MatchString(file) { | ||
file = string(rgxNumericSuffix.ReplaceAll([]byte(file), []byte(""))) | ||
} | ||
// Rewrite paths submitted with an older, buggy client where the Windows | ||
// path is being treated as part of the filename. | ||
file = strings.Replace(file, "\\", "/", -1) | ||
return filepath.FromSlash(file) | ||
} | ||
|
||
type downloadPayload struct { | ||
Solution struct { | ||
ID string `json:"id"` | ||
|
@@ -298,19 +344,6 @@ type downloadPayload struct { | |
} `json:"error,omitempty"` | ||
} | ||
|
||
func (dp downloadPayload) metadata() workspace.ExerciseMetadata { | ||
return workspace.ExerciseMetadata{ | ||
AutoApprove: dp.Solution.Exercise.AutoApprove, | ||
Track: dp.Solution.Exercise.Track.ID, | ||
Team: dp.Solution.Team.Slug, | ||
ExerciseSlug: dp.Solution.Exercise.ID, | ||
ID: dp.Solution.ID, | ||
URL: dp.Solution.URL, | ||
Handle: dp.Solution.User.Handle, | ||
IsRequester: dp.Solution.User.IsRequester, | ||
} | ||
} | ||
|
||
func setupDownloadFlags(flags *pflag.FlagSet) { | ||
flags.StringP("uuid", "u", "", "the solution UUID") | ||
flags.StringP("track", "t", "", "the track ID") | ||
|
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 think this method might be going beyond the scope of the download type. Which If I understand correctly is a custom type used for parsing flags and setting up the download command. Having actual exercise metadata and solutionFile data ⬇️ tied to it doesn't seem correct.