-
Notifications
You must be signed in to change notification settings - Fork 0
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
# Wataru / boru 課題1 #2
base: master
Are you sure you want to change the base?
Conversation
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.
レビューしました!
kadai1/wataboru/.gitignore
Outdated
@@ -0,0 +1,3 @@ | |||
.idea | |||
.DS_Store | |||
go.sum |
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.
ignoreする必要はないですね
package main | ||
|
||
import ( | ||
"dojo8/kadai1/wataboru/imageconverter" |
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.
github.comからのパスの方が好ましいです。
|
||
const ( | ||
// ExitCodeSuccess is the exit code on success | ||
ExitCodeSuccess int = iota |
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.
終了コードは外部に伝えるものなので iota
は使わない方が安全です。
iota
を使う値は区別さえできればOKで値として意味がないものに限ります。
終了コードは0が成功、それ以外が失敗という値に意味を持っているので使わないほうがが良いです。
DBに入れる値なんかも同様です。
kadai1/wataboru/README.md
Outdated
ex) --dir=./convert_image | ||
$ ./imgconv -d ./image | ||
or | ||
$ ./imgconv -d ./image -b png -a gif |
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.
テスト用の画像は testdata
というディレクトリを作ってその下に入れましょう。
testdata
という名前のディレクトリはGoにパッケージとして認識されません。
同様に_
で始まるディレクトリ(よく_example
として使用される)や.
で始まるディレクトリも無視されます。
|
||
- 前提として、@tenntennさんの公開されている[handsonプロジェクト](https://github.com/tenntenn/gohandson/tree/master/imgconv/ja)と似ている内容でして、以前やったことがありました。 | ||
- そちらに無い部分として、tiff変換やbmp変換の追加などを行いました。 | ||
- GoModulesを利用したく、`golang.org/x/image`を導入しました |
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.
👍
"strings" | ||
|
||
"golang.org/x/image/tiff" | ||
|
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 != nil { | ||
return fmt.Errorf("image file could not be created. target: %s", dest) | ||
} | ||
defer destFile.Close() |
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.
Create
は書き込みの処理なので、Close
する際にもエラー処理をする。
func Convert(args Args) error { | ||
return filepath.Walk(args.Directory, func(path string, info os.FileInfo, err error) error { | ||
if err != nil { | ||
fmt.Printf("prevent panic by handling failure accessing a path %q: %v\n", path, 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.
エラーをreturnするならログを出さない。
ログを出すならエラーを返さない。
エラーログの出力もエラーハンドリングの1つでエラーハンドリングは1度だけにする。
出すとしても標準エラー出力の方が好ましそう。
- Modify import statement in main.go. - Add error handling to defer of imageconverter.convertImage.
|
||
defer func(returnErr error) { | ||
if returnErr == nil { | ||
err = destFile.Close() |
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.
@tenntenn
こちら、分かる限り修正してみました。
一点質問がありまして、defer内で呼び出し元にエラーを返す処理を実現するため、名前付き戻り値を使ってしまっています。
他の方のレビューの中で、「何をreturnしたか追いづらいので、名前付き戻り値はできるだけ使わない。」とあったので、名前付き戻り値を使わずにどのように呼び出し元にエラーを返したら良いか教えていただきたいです。
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.
このパターンのみ使わないとどうにもならないので使っても大丈夫です!
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.
@tenntenn
なるほど!そうなんですね。
一応その他の場所は明示的にreturnに渡す様にしました。
課題1のPRになります。レビューをお願いいたします。
以下README.mdより転記
課題 1 【TRY】画像変換コマンドを作ろう
次の仕様を満たすコマンドを作って下さい
以下を満たすように開発してください
実行手順
コメント
golang.org/x/image
を導入しました