Skip to content

リポジトリを指定して全ファイルチェックするスクリプトを追加#33

Merged
sfujiwara merged 11 commits into
masterfrom
issue-32
Nov 18, 2019
Merged

リポジトリを指定して全ファイルチェックするスクリプトを追加#33
sfujiwara merged 11 commits into
masterfrom
issue-32

Conversation

@sfujiwara
Copy link
Copy Markdown
Member

@chie8842 レビュアーが翻訳者のリポジトリを手作業で clone してこなくても良い感じにしてみたので見てもらえると。

ツールが 2 種類ある状態になってしまったので、 README.md どうしようかなあというところがあんまりまとまってなかったりします。

スクリプトを追加

  • bin/run
  • bin/run-docker

次のように使用する:

./bin/run ${GITHUB_REPOSITORY} ${BRANCH} ${OUTPUT_FILE}

素の環境で使う場合は

./bin/run tensorflow/docs master result.txt

Docker を使う場合は

./bin/run-docker tensorflow/docs master result.txt

Dockerfile の修正

  • git が必要だったのでそれをインストールするよう修正

README.md の修正

  • 上記のスクリプトをベースに README.md を修正
    • 初見の人が混乱しないよう元のスクリプトの説明はいったん削除
    • ここはどうしようかちょっと迷ったので意見もらえると (両方書こうとしたけど上手くまとめられなかった)
  • ただし元々あったスクリプトもいったん README.md から除いただけで残してある
    • 個別のファイルチェックなど元のスクリプトにしかない機能があるため

その他

  • 細かい改行が気になったのでついでにちょい修正

@sfujiwara sfujiwara requested a review from chie8842 November 7, 2019 16:03
@sfujiwara sfujiwara self-assigned this Nov 7, 2019
@chie8842
Copy link
Copy Markdown
Collaborator

今からレビューします!

@chie8842
Copy link
Copy Markdown
Collaborator

#16
これの対応(htmlファイルのチェック)はできなくていいでしょうか?

@chie8842
Copy link
Copy Markdown
Collaborator

1ファイルだけチェックする機能もなくすかんじですかね?
補完きかない問題があったし割り切るならそれはそれでいいと思います!

@chie8842
Copy link
Copy Markdown
Collaborator

もとのスクリプトでも発生しうる問題で気づいたことなのですが、同じディレクトリ配下に同じ名前と異なる拡張子をもつファイルがあった場合に上書きが起こってしまいますね。
一応今のところはそういうものがないことは確認しました。

Comment thread bin/run
echo "GITHUB_REPOSITORY: ${GITHUB_REPOSITORY}"
echo "GITHUB_REPOSITORY_URL: ${GITHUB_REPOSITORY_URL}"
echo "BRANCH: ${BRANCH}"
echo "OUTPUT_FILE: ${OUTPUT_FILE}"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

細かいですが、引数の数を減らす意味で、OUTPUT_FILEのファイル名は固定でもいいのかなと思いました。

@chie8842
Copy link
Copy Markdown
Collaborator

ツールが 2 種類ある状態になってしまった

上述した機能差異を新しいスクリプトに取り込むか割り切ってしまって、削除するほうが管理が楽かなと思いました。

Copy link
Copy Markdown
Collaborator

@chie8842 chie8842 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
Copy link
Copy Markdown
Member Author

必要に応じて昔のコミット掘り出せば良いので、ひとまず必要最低限のもの以外削除して一本化する感じでいきましょうか
ついでに移植の issue 立てても良いかも

@chie8842
Copy link
Copy Markdown
Collaborator

👍

@sfujiwara sfujiwara merged commit 129abf6 into master Nov 18, 2019
@sfujiwara sfujiwara deleted the issue-32 branch November 22, 2019 13:53
@chie8842 chie8842 mentioned this pull request Nov 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants