yumemi-inc / flutter-mobile-project-template

MIT License
37 stars 7 forks source link

PR のフォーマットをチェックするフローを追加 #64

Closed trm11tkr closed 10 months ago

trm11tkr commented 10 months ago

概要

close: #8

レビュー観点

レビューレベル

レビュー優先度

画像 / 動画

なし

備考

github-actions[bot] commented 10 months ago

Ready for review :rocket:

trm11tkr commented 10 months ago

@morikann @warahiko レビューよろしくお願いいたします🙏

trm11tkr commented 10 months ago

@morikann @warahiko ご指摘いただいた箇所を修正いたしました。お手隙の際に再度レビューお願いいたします🙇‍♂️ 特に見ていただきたいコミットは以下になります。

trm11tkr commented 10 months ago

@warahiko ご指摘いただいた箇所を修正いたしましたので再度レビューお願いいたします🙏

trm11tkr commented 10 months ago

@warahiko

一点、format.sh にあって check-format-ci.sh にない -path "./.dart_tool/*" が気になりましたが、 action の結果では問題起きてないので大丈夫ですかね? ローカルで試すとここが失敗扱いになるのですが、多分ビルドしてなければ問題ない、ってところでしょうか

check-format-ci.sh は Action でのみ使用される想定でしたので、-path "./.dart_tool/*" の指定は不要だと判断いたしました…!

trm11tkr commented 10 months ago

レビューいただきありがとうございました! こちらマージさせていただきます!

warahiko commented 10 months ago

check-format-ci.sh は Action でのみ使用される想定でしたので、-path "./.dart_tool/*" の指定は不要だと判断いたしました…!

了解です!

Approve したあとに調べてたんですが、.dart_tool の内部ファイルの生成タイミング、

みたい?なので、例えば check-pr.yaml で Check format と Run test の順序を入れ替えるだけで落ちるようになる気もしました 現状問題ではないですが、今後気をつけないといけないくらいなら今のうちにパッとやってしまってもいいかもしれないですね

と書いてますが、この PR 自体はマージでよいと思います〜!

trm11tkr commented 10 months ago

例えば check-pr.yaml で Check format と Run test の順序を入れ替えるだけで落ちるようになる気もしました

サンプルプロジェクトを作成して確認すると失敗していました…。なので今後の懸念をなくすためにも今修正してしまったほうが良さそうですね…! 8e7dac9 にて修正いたしました!

@warahiko 何度も申し訳ありませんが、レビューお願いいたします…!🙇‍♂️

trm11tkr commented 10 months ago

お二方とも丁寧にレビューしていただきありがとうございました! マージさせていただきます!