Skip to content
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

Fix issue 25 md5s #26

Merged
merged 10 commits into from
Jun 1, 2021
Merged

Fix issue 25 md5s #26

merged 10 commits into from
Jun 1, 2021

Conversation

KEINOS
Copy link
Member

@KEINOS KEINOS commented May 31, 2021

No description provided.

@KEINOS KEINOS marked this pull request as ready for review May 31, 2021 02:48
@KEINOS
Copy link
Member Author

KEINOS commented May 31, 2021

Issue #25 を修正してみました。

md5s だけでなく md5f も同じ問題を孕んでますが、

Issue 発生 → Draft PR → 再現テスト(test: failure) → 不具合修正(test: success)→ 修正の反映 → PR

の流れを確認してみたかったので、md5s の修正に限定してみました。

お手すきにチェックお願いします。

@yoshi389111
Copy link
Collaborator

確認しました。修正箇所は問題ないと思います。

1つ気になるのは、どうやらテストコード issue25_test.sh が bash ではなく sh で動いているように思えることです。

たとえば

md5s() {
    if [ -e "$(which md5sum)" ]; then
        md5sum <(echo "$1") | awk '{ print $1 }'
    # 以下略

のように、bash ではつかえるけど sh では使えない書き方( <(echo "$1") )を使っていると、エラーになります。
.github/run-test.sh で実行しても同様です。

shellspec --shell bash とすればテストは通ります。
同様に .shellspec--shell bash を足した場合もテストは通ります。

(テストケース1行目の shellcheck shell=bash が効いていない?)

@KEINOS
Copy link
Member Author

KEINOS commented Jun 1, 2021

確認しました。修正箇所は問題ないと思います。

確認あざっす!

確認ですが、こういった、なんちゃって TDD 的なフローはレビューしやすいですか?

issue25_test.sh が bash ではなく sh で動いているように思える

そうなんですよ!おっしゃるように、--shell bash と明示しないとダメらしく、テストケースのヘッダー行が無視されているようです。

bash ではつかえるけど sh では使えない書き方

確かに他のスクリプトはプロセス置換(?)ではなくパイプ渡し(echo "$1" | md5sum | )の書き方をしているので、合わせた方がいいですよね。修正します。

せっかく shellspec を使っているので、最終的には Bash 中心でなく POSIX 準拠にしたいのですが、とりあえず今は Bash で進めて、別 Issue をあげたいと思います。

@yoshi389111
Copy link
Collaborator

なんちゃって TDD 的なフローはレビューしやすいですか?

うーん、どうでしょう。(役に立たない意見)

修正意図はわかりやすいかもしれないですね。

確かに他のスクリプトはプロセス置換(?)ではなくパイプ渡し(echo "$1" | md5sum | )の書き方をしているので、合わせた方がいいですよね。修正します。

いえ、直した方が良いという意味ではありません。
いろいろ確認していたら、bash ではないようだったので、説明上書き換えて、確認しただけです。
今のままで問題ないと思います。

最終的には Bash 中心でなく POSIX 準拠にしたいのですが

POSIX 準拠の方が移植性は良いだろうと思いますので、賛成です。

@emadurandal
Copy link
Contributor

@KEINOS @yoshi389111 わわ、動かなかったですか。すみませんMacでの確認が足りなかったみたいですね💦

最終的には Bash 中心でなく POSIX 準拠にしたいのですが
POSIX 準拠の方が移植性は良いだろうと思いますので、賛成です。

たしかにそうですね。私があまりシェルスクリプトに慣れていないもので、今後も私のコントリビュートについてはご面倒をおかけするかもしれません(勉強します)

@KEINOS
Copy link
Member Author

KEINOS commented Jun 1, 2021

わわ、動かなかったですか。

いやいや! ✋

十分な確認をせずにマージした私の落ち度なんです。私のアイコンをご覧ください。

ね?ザル🐒 なんですw。なので、テストを入れようとしているのは、私のポカよけの意味が多分にあるし、見つけられただけ良いことで直せばいいので気にしないでください。むしろ #5 の PR のおかげで重い腰が動いたのです。ありがとうございます。

POSIX 準拠の方が移植性は良いだろうと思いますので、賛成です。

POSIX 互換にしたいのは Docker で作業することが多くなったので、そこでも使えるとカッチョいいなというだけ(本音)で、大義としては、POSIX 準拠したスクリプトであればより幅広い人にも使えるかな、と。カッチョいいし。

とりあえず、変更せずに進めて、別 Issue 別 PR で POSIX 準拠を進めたいと思います。

各々がたチェックありがとうございます!

@KEINOS KEINOS force-pushed the fix-issue-25-md5s branch from 33441da to 2b8dbbb Compare June 1, 2021 13:02
@KEINOS
Copy link
Member Author

KEINOS commented Jun 1, 2021

とりあえず、テスト時のシェルを bash にするように .shellspec ファイルに追記しました。

@KEINOS KEINOS merged commit f7e34f9 into Qithub-BOT:master Jun 1, 2021
@KEINOS KEINOS deleted the fix-issue-25-md5s branch June 1, 2021 13:16
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.

3 participants