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

Feature/replace cleanup workflow with cli app #62

Merged
merged 49 commits into from
Mar 21, 2024

Conversation

morikann
Copy link
Contributor

@morikann morikann commented Mar 13, 2024

提出前チェックリスト (必須)

  • 貢献ガイド を読み、そこに記載されているプロセスに従って PR を送信しました。
  • この PR によって修正される Issue を少なくとも 1 つ挙げました。または簡単な修正。
  • 関連ドキュメントを更新・追加しました。

概要

また、問題なくアプリが起動することも確認しました。

iOS android

Issue(オプション)

close #41

@morikann morikann changed the title Feature/replace cleanup workflow with cli Feature/replace cleanup workflow with cli app Mar 13, 2024
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.

@morikann
いったんいくつかコメントしましたので、ご確認お願いします 🙏

tools/cleanup_template/lib/src/file_system.dart Outdated Show resolved Hide resolved
tools/cleanup_template/lib/cleanup_template.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@Yamasaki-pan961 Yamasaki-pan961 left a comment

Choose a reason for hiding this comment

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

対応ありがとうございます。
責務分割などだいたい良いと思いました。

いくつかコメントしましたのでご確認お願いします。

tools/cleanup_template/README.md Show resolved Hide resolved
tools/cleanup_template/lib/cleanup_template.dart Outdated Show resolved Hide resolved
tools/cleanup_template/build.yaml Show resolved Hide resolved
tools/cleanup_template/lib/src/file_system.dart Outdated Show resolved Hide resolved
Comment on lines +166 to +173
extension _ProcessResultExt on ProcessResult {
void throwExceptionIfFailed() {
final exitStatus = ExitStatus.fromCode(exitCode);
if (exitStatus != ExitStatus.success) {
throw ProcessRunException(exitStatus, stdout.toString());
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Process.runSync 自体は例外を出さないということですよね?
良さそうです

@morikann
Copy link
Contributor Author

FB 対応後のコードで問題なく CLI が動いていることを確認しました。
https://github.com/morikann/flutter-training-sample

@blendthink
Copy link
Member

blendthink commented Mar 19, 2024

@morikann
VSCode の settings.json の記載を修正しないといけなさそうですので、ご確認お願いします 🙏
https://github.com/morikann/flutter-training-sample/blob/7cd5a19ff8da132e2b612a6eecb2a25baff6f751/.vscode/settings.json#L2

@morikann
Copy link
Contributor Author

@blendthink
⇩で修正致しました🙏
6216a14

実行結果は⇩となります!
https://github.com/morikann/sample_workflow5/blob/main/.vscode/settings.json

);
_fileSystem.file(analysisOptionsPath).writeAsStringSync('''
# https://pub.dev/packages/yumemi_lints
include: package:yumemi_lints/flutter/$flutterVersion/recommended.yaml
Copy link
Member

@blendthink blendthink Mar 19, 2024

Choose a reason for hiding this comment

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

memo-badge
yumemi_lints 2.0.0 への対応について Issue を作成しておきました。
#63

Comment on lines +32 to +35
Process.runSync(
'dart',
['pub', 'global', 'activate', 'fvm'],
).throwExceptionIfFailed();
Copy link
Member

Choose a reason for hiding this comment

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

memo-badge
実行ファイルを生成して、それを利用する場合は Homebrew 経由で取得する必要がありそう。
#64

Comment on lines +48 to +51
Process.runSync(
'fvm',
['install', flutterVersion.toString(), '-s'],
).throwExceptionIfFailed();
Copy link
Member

Choose a reason for hiding this comment

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

memo-badge
fvm 使わなくても対応できるかもしれないと思ったので Issue 作成しました。
考慮漏れているかもしれないので、このプルリクエストでは対応しない方針で大丈夫です、、!
#65

@morikann morikann force-pushed the feature/replace_cleanup_workflow_with_CLI branch from 5c12fd7 to 02b593f Compare March 19, 2024 10:51
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 させていただきます!
ご対応ありがとうございます 🙏

LGTM

.github/templates/.vscode/settings.json Outdated Show resolved Hide resolved
Copy link
Contributor

@Yamasaki-pan961 Yamasaki-pan961 left a comment

Choose a reason for hiding this comment

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

ご対応ありがとうございました!LGTM

@morikann morikann merged commit 2727d8e into main Mar 21, 2024
@morikann morikann deleted the feature/replace_cleanup_workflow_with_CLI branch March 21, 2024 06:00
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.

[Template]:InitWorkFlow内の一時ディレクトリ名HardCodeはやめた方がいい気がします
3 participants