Skip to content

Conversation

@chie8842
Copy link
Collaborator

@chie8842 chie8842 commented Dec 1, 2019

@sfujiwara スクリプトですが、なるべく重複部分をなくしたかったので、
わたしがこうしたいな、と思うように少し変えてみました!
どうでしょうか?

修正点:

  • d43899b 397363d
    1. bin/run をbin/run-allに、bin/diffをbin/run-diffに名称変更
    2. bin/diffとbin/runで重複する部分を切り出してrunスクリプトとして、runスクリプトからrun-diffもしくはrun-allスクリプトを呼び出すようにする
    3. スクリプトの親子関係がわかりやすいようにヘッダ追加
    4. OUTPUT_FILEはここのoutputと表現がかぶるのでLOG_FILEに名称変更し、ファイル名は固定にする
  • 2c6c85b README修正

@sfujiwara sfujiwara self-requested a review December 17, 2019 16:40
@sfujiwara sfujiwara added the enhancement New feature or request label Dec 17, 2019
@sfujiwara
Copy link
Member

設計方針とかざっくりコード見た感じは賛成です!
#37 #39 の前にこっちをマージした方が良さそうなので、先にこっち見ますね
細かい動作確認とかはまだなので、少々お待ちを

Copy link
Member

@sfujiwara sfujiwara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ash が明示的に指定されていて Docker を使わずに直接 ./bin/run を実行した時に動かないので

  • Suggestion のような感じで #!/bin/sh で指定する
  • Docker を使った実行のみサポートする方針にする
    • この場合は README.md から "Without Docker" の項目を削除する

のいずれかの対応が必要そうです。
どちらの方針もありだと思うので、好きな方を選んじゃって良いかと。

その他の動作確認は大丈夫そうでした。

@sfujiwara
Copy link
Member

メンションをつけ忘れていた
@chie8842 だいぶ遅くなっちゃいましたが、レビューしましたー

chie8842 and others added 6 commits December 21, 2019 17:30
Co-Authored-By: Shuhei Fujiwara <shuhei.fujiwara@gmail.com>
Co-Authored-By: Shuhei Fujiwara <shuhei.fujiwara@gmail.com>
Co-Authored-By: Shuhei Fujiwara <shuhei.fujiwara@gmail.com>
Co-Authored-By: Shuhei Fujiwara <shuhei.fujiwara@gmail.com>
Co-Authored-By: Shuhei Fujiwara <shuhei.fujiwara@gmail.com>
@chie8842 chie8842 force-pushed the feature/diff-chie8842 branch from 5afb344 to dd04378 Compare December 21, 2019 08:59
@chie8842
Copy link
Collaborator Author

ありがとうございます!suggestion反映したのと、READMEを少し修正しました🙏

Copy link
Member

@sfujiwara sfujiwara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sfujiwara sfujiwara merged commit c682d4f into feature/diff Dec 22, 2019
@sfujiwara sfujiwara deleted the feature/diff-chie8842 branch December 22, 2019 07:11
@sfujiwara
Copy link
Member

あああ!
これ feature/diff の方にマージしていたのかああ!
master にマージしたと思って、ミスって feature/diff を消すというやらかしをしてしもうた...

@chie8842 chie8842 restored the feature/diff-chie8842 branch December 22, 2019 08:06
@chie8842 chie8842 mentioned this pull request Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants