-
Notifications
You must be signed in to change notification settings - Fork 7
Kadai3-2: lfcd85 #33
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
base: master
Are you sure you want to change the base?
Kadai3-2: lfcd85 #33
Conversation
kadai3-2/lfcd85/Makefile
Outdated
| PHONY: fmt | ||
| fmt: | ||
| go fmt ./... | ||
| go vet ./... |
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.
実は最近はgo testで自動でかかる
kadai3-2/lfcd85/mypget/mypget.go
Outdated
| } | ||
|
|
||
| // Execute do the split download. | ||
| func (d *Downloader) Execute() error { |
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.
せっかくなので引数にcontext.Contextとって、nilならBackgroundにしても良いかも。
kadai3-2/lfcd85/mypget/mypget.go
Outdated
| ctx, cancel := context.WithCancel(bc) | ||
| defer cancel() | ||
|
|
||
| req, err := http.NewRequest("GET", d.url.String(), nil) |
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.
http.MethodGet
| return err | ||
| } | ||
|
|
||
| resp, err := http.DefaultClient.Do(req) |
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.
HTTPクライアントを変更できるようにしておくとよいかも。
最近はあんまり必要ないかもしれないけど、GAEだと特殊なHTTPクライアントを使う必要があったりするので。
kadai3-2/lfcd85/mypget/mypget.go
Outdated
| return err | ||
| } | ||
|
|
||
| err = d.combine(tempDir) |
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.
if err := ...にしてスコープを小さくする
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Printf("Download completed! saved at: %v\n", d.outputPath) |
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.
出力先を変更できるようにする
kadai3-2/lfcd85/mypget/mypget.go
Outdated
| } | ||
| fmt.Printf("Download completed! saved at: %v\n", d.outputPath) | ||
|
|
||
| return err |
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.
nilを返す
kadai3-2/lfcd85/mypget/mypget.go
Outdated
| } | ||
|
|
||
| func (d *Downloader) downloadByRanges(ctx context.Context, tempDir string) error { | ||
| var eg errgroup.Group |
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.
errgourp.WithContextを呼ばないとキャンセルされない
| return err | ||
| } | ||
|
|
||
| partialPath := generatePartialPath(tempDir, i) |
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.
MEMO
tempDirは読み込みだけなのでセーフ
|
|
||
| for i, _ := range d.ranges { | ||
| partialPath := generatePartialPath(tempDir, i) | ||
| partial, err := os.Open(partialPath) |
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.
閉じてない
|
|
||
| w.Header().Set("Content-Length", strconv.Itoa(len(body))) | ||
| w.WriteHeader(http.StatusPartialContent) | ||
| w.Write(body) |
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.
エラー処理
| return ts, func() { ts.Close() } | ||
| } | ||
|
|
||
| func (tsf *testServerFile) testServerHandler(t *testing.T, w http.ResponseWriter, r *http.Request) { |
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.
せっかくなんで大きなファイルもテストできるようにするとよいかも。
課題3-2
分割ダウンローダを作ろうの実装です。Specs
TODO
testdataからテストできるようにしました: 0dd26ec(
hioki-daichiさんのこちらを参考にしました)