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

dart fix --dry-run での確認フローを追加 #113

Merged
merged 9 commits into from
Feb 13, 2024

Conversation

trm11tkr
Copy link
Contributor

@trm11tkr trm11tkr commented Feb 4, 2024

概要

close #72

レビュー観点

レビューレベル

  • Lv1: ぱっとみて違和感がないかチェックして Approve する
  • Lv2: 仕様レベルまで理解して、仕様通りに動くかある程度検証して Approve する
  • Lv3: 実際に環境で動作確認したうえで Approve する

レビュー優先度

  • すぐに見てもらいたい ( hotfix など ) 🚀
  • 今日中に見てもらいたい 🚗
  • 今日〜明日中で見てもらいたい 🚶
  • 数日以内で見てもらいたい 🐢

画像 / 動画

動作変更なし

Before After Design

動作確認手順

動作変更なし

備考

参考

https://gist.github.com/hell0again/f5f3064ad1ea6c5c7308

scripts/check-format-ci.sh Outdated Show resolved Hide resolved

PIDS=()
for file in "${files[@]}"; do
# limit jobs to 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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


(
# Note: The script relies on the 'Nothing to fix!' output from 'dart fix --dry-run'.
# This dependency might be an issue if the output format changes in future Dart versions.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

memo-badge
dart fix --dry-run の出力の最後が 「Nothing to fix!」で終了しているかで判定

背景

dart fix --applyを実行し、git diff による差分で比較しない理由

実行速度において、dart fix --dry-rundart fix --applyより遥かに速いため

「Nothing to fix!」という文言で判定している理由

dart fix --dry-runは修正必要なファイルを検出してもステータスコード 0 を返すため

FYI: dart fix --dry-run の実行結果

修正必要なファイルなし

fvm dart fix --dry-run  
Computing fixes in flutter-mobile-project-template (dry run)...
Nothing to fix!

修正必要なファイルを検出

fvm dart fix --dry-run
Computing fixes in flutter-mobile-project-template (dry run)...

1 proposed fix in 1 file.

apps/app/lib/main.dart
  directives_ordering - 1 fix

To fix an individual diagnostic, run one of:
  dart fix --apply --code=directives_ordering 

To fix all diagnostics, run:
  dart fix --apply 

# Note: The script relies on the 'Nothing to fix!' output from 'dart fix --dry-run'.
# This dependency might be an issue if the output format changes in future Dart versions.
if ! dart fix --dry-run "$file" | grep -q "Nothing to fix!"; then
echo "Fixes might be needed for $file"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

memo-badge
dart fix --dry-runの実行ログが表示されないようになるため、修正必要ファイルが検出された際にはメッセージを表示

@trm11tkr trm11tkr marked this pull request as ready for review February 5, 2024 03:26
@trm11tkr trm11tkr changed the title [WIP] dart fix --dry-run での確認フローを追加 dart fix --dry-run での確認フローを追加 Feb 5, 2024
Copy link

github-actions bot commented Feb 5, 2024

Ready for review 🚀

@trm11tkr
Copy link
Contributor Author

trm11tkr commented Feb 5, 2024

@Kotaro666-dev @warahiko
レビューよろしくお願いします🙏

Copy link
Contributor

@warahiko warahiko left a comment

Choose a reason for hiding this comment

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

全体的にはよさそうに感じました 👍
少し細かい部分コメントしています
(sh の経験あんまりないので変なこと言ってたら指摘してください 🙏 )

scripts/check-format-ci.sh Outdated Show resolved Hide resolved
scripts/check-format-ci.sh Outdated Show resolved Hide resolved
scripts/check-format-ci.sh Show resolved Hide resolved
Comment on lines 13 to 15
find_files() {
find . -name "*.dart" -not \( -name "*.freezed.dart" -o -name "*.g.dart" -o -name "*.gen.dart" -o -path "*/gen/*.dart" -o -path "./.dart_tool/*" \) -print0
}
Copy link
Contributor

Choose a reason for hiding this comment

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

comment
このあたり format.sh でもありますし、管理上漏れが出そうな見た目してるので共通化したほうがいいかもしれないですね

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4c024f6にて共通化しました。

Copy link
Contributor

Choose a reason for hiding this comment

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

common.sh というのが一般的すぎる気もしますが、今のところ問題ないかなと思いました 👍
( $0 より適当な書き方があるんですね......!勉強なります)

scripts/check-format-ci.sh Outdated Show resolved Hide resolved
scripts/check-format-ci.sh Outdated Show resolved Hide resolved
scripts/check-format-ci.sh Outdated Show resolved Hide resolved
@trm11tkr
Copy link
Contributor Author

trm11tkr commented Feb 9, 2024

@warahiko @Kotaro666-dev
こちら再度レビューよろしくお願いします 🙇‍♂️

Copy link
Contributor

@warahiko warahiko left a comment

Choose a reason for hiding this comment

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

細かいところまでご対応ありがとうございました! 👍

@trm11tkr
Copy link
Contributor Author

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

@trm11tkr trm11tkr merged commit fc7ebce into main Feb 13, 2024
3 checks passed
@trm11tkr trm11tkr deleted the feature/add_check_flow_of_dart_fix_on_ci branch February 13, 2024 06:42
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.

[Improve]: Pull Request チェックに dart fix --dry-runでの確認フローを追加する
3 participants