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

PRテンプレートで変更対象とカテゴリを分けて書くようにする #1814

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 2, 2022

PR の目的

PRの変更対象と内容のカテゴリを分離します。

カテゴリ

  • GitHub 関連の問題

PR の背景

カテゴリは用意された選択肢の中で必要なものを残すタイプですが
「アプリの不具合修正」と明記するか、ビルド関連のなかに「不具合修正」を用意しておいてほしいですね。
不具合修正をその他と書くのは抵抗を感じます・・・。

Originally posted by @dep5 in #1806 (comment)

現状は「何に対する」カテゴリなのかよくわからないので、レビュー過程で「その選択は間違っている」と言われることがあります。

PR のメリット

「何」に対して「どんな変更」をしたいかを書きやすくなります。

PR のデメリット (トレードオフとかあれば)

仕様・動作説明

PR の影響範囲

  • pull request template

テスト内容

関連 issue, PR

@ghost
Copy link
Author

ghost commented Mar 2, 2022

ということで、 @dep5 さんにも意見を求めておきます。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

変更は問題ないように思いました。

- リファクタリング
- ドキュメント修正
- プログラムの動作上の問題
- サクラエディタ本体
- 正式リリース版
- Azure Pipelines ビルド版
- AppVeyor ビルド版
- GitHub Actions ビルド版
- ローカルビルド版
Copy link
Contributor

Choose a reason for hiding this comment

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

「~ビルド版」って要らないですね、たぶん(指摘ではないです。

Copy link
Author

Choose a reason for hiding this comment

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

「ビルド」は確かに要らないですね。

ところで、バージョン情報ダイアログ以外にCIサービスの違いが動作に影響しそうな箇所はない気がするのですが、区別したほうがいいのでしょうか?
こういう書き方になったのは #914 からのようですが、これには理由が書かれていません。

Copy link
Contributor

Choose a reason for hiding this comment

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

わからないです。

「Appveyorだけに影響する修正」とか「Azure Pipelinesだけに影響する修正」とかあったので作ったんだと思います。

よくよく考えてみるとそれは「CIスクリプトの不具合修正orプロセス改善」であるような気がします。

「ビルド環境の違いがCDlgAboutに影響する」というのも実は「githash.batの出力結果はビルド環境によって変わる」に依存する結合テスト的影響の産物なので、「何」を修正して「どこ」に影響するか?の「どこ」を分かりやすくするためのものだったと考えられます。

で、結局「影響範囲」に書ける内容なので「カテゴリ」に「~ビルド版」があるのはおかしいように思います。

- 正式リリース版
- Azure Pipelines ビルド版
- AppVeyor ビルド版
- GitHub Actions ビルド版
- ローカルビルド版
- ヘルプ
- インストーラ
- ビルド関連
- ビルド手順
- Azure Pipelines
- AppVeyor
- GitHub Actions
- ローカルビルド
Copy link
Contributor

Choose a reason for hiding this comment

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

  • MSVCビルド(全ビルド環境に影響)
  • MinGWビルド(現状はAzpのみに影響)
  • 本体以外のビルド(全ビルド環境に影響)

あたりを追加したら分かりやすくなる気がしました(指摘ではないです。


- 機能追加
- 仕様変更
- 不具合修正
Copy link
Contributor

Choose a reason for hiding this comment

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

「不具合修正」を小分けにできないか?ということを考えていました。

  • アプリ不具合修正
    • 仕様変更あり
    • 仕様変更なし
  • その他不具合修正
    • 仕様変更あり
    • 仕様変更なし

もっと小分けにできるかも・・・(指摘ではないです。

- 仕様変更
- 不具合修正
- 速度向上
- リファクタリング
Copy link
Contributor

Choose a reason for hiding this comment

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

「速度向上」と「リファクタリング」はレベルが違うのが前から気になっていました。

  • 速度向上 修正の目的「速度向上」を表す。
  • リファクタリング 修正の手段「リファクタリング」を表す。

「 速度向上」させる手段「リファクタリング」は有効です。

ニュアンス伝わりますかね?
2つの単語の意味するものの粒度というか種類が違うんです。。。

結論が出ないので、これも指摘ではないです。

Copy link
Author

Choose a reason for hiding this comment

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

変更目的は「PRの目的」欄に記述され、そのためにどんな手段をとるのかがここに来るものと考えました。
なので…ここに「速度向上」が残っているのはちょっと変でしたね。

Copy link
Contributor

@berryzplus berryzplus Mar 12, 2022

Choose a reason for hiding this comment

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

  • 「プログラム構造が煩雑なのを分かりやすくしたい」が目的だとして、手段「リファクタリング」はアリ。
  • 「~したときに~となるのが困るので対処したい」が目的だとして、手段「仕様変更」はアリ。
  • 「~したときに~となるのが困るので対処したい」が目的だとして、手段「不具合修正」はアリ。
  • 「~したときにアプリが固まって困るので対処したい」が目的だとして、手段「速度向上」は意味不明。

「カテゴリ」は「手段である」と考えるとしっくりきそうです。

@ghost ghost marked this pull request as draft March 11, 2022 04:02
@ghost
Copy link
Author

ghost commented Mar 11, 2022

@kazasaku TODO: issue templates

- 不具合修正
- 速度向上
- リファクタリング
- ドキュメント修正
- GitHub 関連の問題
- 実験 (master へのマージを目的としない)
- その他の問題
Copy link
Contributor

Choose a reason for hiding this comment

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

これも指摘ではないですが「カテゴリは手段である」と置いたとき、
「GitHub関連の問題」と「その他の問題」が意味不明になります

*「~したときに~となるのが困るので対処したい」が目的だとして、手段「GitHub関連の問題」は意味不明。
*「~したときに~となるのが困るので対処したい」が目的だとして、手段「実験 (master へのマージを目的としない)」はアリ。
*「~したときに~となるのが困るので対処したい」が目的だとして、手段「その他の問題」は意味不明。

issue templateを導入した時のスタンスが「とりあえず入れてみようぜ!」で中身を真面目に見てなかったことが原因と思います。

@berryzplus
Copy link
Contributor

ん?「カテゴリは手段である」と置くと「サクラエディタ本体」と「ビルド関連」も意味不明になるような(笑

@ghost ghost closed this May 5, 2022
@ghost ghost deleted the feature/add_target_to_pr_template branch May 5, 2022 05:20
This pull request was closed.
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.

1 participant