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.

Sign up for GitHub

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

Jump to bottom

ci(melos): スクリプトの命名変更 #357

Merged
merged 1 commit into from
Jan 5, 2025
Merged

ci(melos): スクリプトの命名変更 #357

merged 1 commit into from
Jan 5, 2025
+20 −19

Conversation

tatsutakein
Copy link
Member

@tatsutakein tatsutakein commented Jan 5, 2025 •
edited
Loading

概要

melos.yaml のスクリプトの命名を簡潔でわかりやすくなるよう変更します。

レビュー観点

  • 命名に違和感がないか
  • 変更の過不足がないか
    • この PR でのスコープは命名変更のみとして、内容のカイゼン(stepsへの変更など)は実施しない想定です

レビューレベル

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

レビュー優先度

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

画像 / 動画

  • 見た目に関する変更がないため省略します。

確認したこと

  • ローカルで各スクリプトが実行できること

tatsutakein
tatsutakein commented Jan 5, 2025
View reviewed changes
melos.yaml
Comment on lines +95 to 98
# @deprecated `melos format` で代用可能
# ローカルでフォーマットとdart fix --applyを実行する
format_and_fix:
exec: bash "$MELOS_ROOT_PATH/scripts/format.sh"
Copy link
Member Author

Choose a reason for hiding this comment

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

memo-badge

melos のバージョンがあがったことにより、独自で定義しなくても良くなった認識です。
#350 にて対応予定です。

Copy link
Member

@blendthink blendthink Jan 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nits-badge
対象ファイルから自動生成ファイルを除外するようにしていたり、dart fix を実行していたりしていたので、完全に代用するのはできないと思います、、！

Copy link
Member Author

Choose a reason for hiding this comment

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

ありがとうございます！
おっしゃるとおり、完全な代替ではないですね…！

ask-badge

そもそもの？疑問になってしまうのですが、#219 にて CI で利用されていたこのスクリプトを melos format コマンド側に置き換えているみたいで、こちらのスクリプトはまだ必要そうなのかが少し悩ましいかもです…？
個人的には format については melos format に任せてしまって、fix 系のスクリプトを別途用意するほうがきれいかなと思ったりしています 👀

Copy link
Member

Choose a reason for hiding this comment

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

個人的には format については melos format に任せてしまって、fix 系のスクリプトを別途用意するほうがきれいかなと思ったりしています 👀

LGTM です！

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 5, 2025

Visit the preview URL for this PR (updated for commit adadf5e):

https://flutter-mobile-project-template-catalog--pr357-feature-odugyyox.web.app

(expires Sun, 12 Jan 2025 11:17:45 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 9ea56735a63d07a7cfe62eb204b0528284c37c23

tatsutakein
tatsutakein commented Jan 5, 2025
View reviewed changes
melos.yaml
exec: bash "$MELOS_ROOT_PATH/scripts/report-custom-lint-ci.sh"
packageFilters:
dependsOn: custom_lint

regenerate_code: melos run regenerate_by_using_gen_l10n && melos run regenerate_by_using_build_runner
gen: melos run gen:l10n && melos run gen:build
Copy link
Member Author

Choose a reason for hiding this comment

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

memo-badge

steps への変更などは #350 にて実施予定です。

tatsutakein
tatsutakein commented Jan 5, 2025
View reviewed changes
docs/ja/NEW_FEATURE.md
@@ -118,7 +118,7 @@ const settingShellBranch = TypedStatefulShellBranch<SettingShellBranch>(
`./apps/app`内で以下のコマンドを実行しnavigatorとappのコード生成を行う。

```shell
melos run regenerate_by_using_build_runner --no-select
melos run gen:build --no-select
Copy link
Member Author

Choose a reason for hiding this comment

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

memo-badge

関連箇所を併せて変更しています。

@tatsutakein tatsutakein marked this pull request as ready for review January 5, 2025 11:21
@yumemi-team-review-requester yumemi-team-review-requester bot requested review from a team and blendthink and removed request for a team January 5, 2025 11:21
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 5, 2025

Ready for review 🚀

@tatsutakein tatsutakein enabled auto-merge January 5, 2025 11:40
blendthink
blendthink reviewed Jan 5, 2025
View reviewed changes
Copy link
Member

@blendthink blendthink left a comment

Choose a reason for hiding this comment

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

@tatsutakein
ドキュメントの修正もありがとうございます！
一点だけコメントさせていただきましたのでご確認お願いいたします🙏

@blendthink blendthink disabled auto-merge January 5, 2025 13:38
@blendthink blendthink enabled auto-merge January 5, 2025 13:38
blendthink
blendthink approved these changes Jan 5, 2025
View reviewed changes
Copy link
Member

@blendthink blendthink left a comment

Choose a reason for hiding this comment

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

気になるところは認識合わせできたので LGTM です！
ご対応ありがとうございます！

@blendthink blendthink merged commit 45fd01f into main Jan 5, 2025
19 checks passed
@blendthink blendthink deleted the feature/GH-352 branch January 5, 2025 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blendthink blendthink blendthink approved these changes

Assignees

@tatsutakein tatsutakein

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improve]: melos.yaml のスクリプトの命名変更
2 participants
@tatsutakein @blendthink