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

バージョン定義を一箇所で定義する #195

Merged

Conversation

m-tmatma
Copy link
Member

バージョン定義を一箇所で定義する

#153#168 を容易に実施するための準備です。
この PR ではバージョン自体は変更しません。

@m-tmatma m-tmatma added the refactoring リファクタリング 【ChangeLog除外】 label Jun 30, 2018
@m-tmatma m-tmatma added this to the next release milestone Jun 30, 2018
@m-tmatma
Copy link
Member Author

version.h の VER_D の値を ローカルで 1 に変えたときのスクリーンショットです。
(この PR では バージョンは変えていません)

test-version1

test-version2

#define VER_A 2
#define VER_B 3
#define VER_C 2
#define VER_D 0
Copy link
Member

Choose a reason for hiding this comment

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

A, B, C, D という命名は仮という認識で良いですかね?
バージョン番号ポリシーが決まったらここの定数はもっとそれらしい名前(VER_MAJOR とか)に変えるという認識で合ってますか?

Copy link
Member

Choose a reason for hiding this comment

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

VER_A 定義の上あたりに、バージョン番号を示すコメントを入れてほしいです。

// 2.3.2.0

みたいな感じのものを。

Copy link
Member Author

Choose a reason for hiding this comment

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

VER_A 定義の上あたりに、バージョン番号を示すコメントを入れてほしいです。

同じ定義を書くという意味ですか?
それだとコメントとマクロ値の両方を変更しないといけなくなりませんか?

Copy link
Member Author

Choose a reason for hiding this comment

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

A, B, C, D という命名は仮という認識で良いですかね?

このままでもいいですし、後で変えてもいいと思います。
VER_MAJOR, VER_MINOR の後の名前が難しいです。
VER_PATCH?, VER_REVISON (VER_BUILD?)

Copy link
Member

@kobake kobake Jun 30, 2018

Choose a reason for hiding this comment

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

同じ定義を書くという意味ですか?
それだとコメントとマクロ値の両方を変更しないといけなくなりませんか?

やや重複があっても人間にとって見やすいほうが自分は嬉しいです。

複数ファイルにまたがる変更が発生するとすると手間に加えてミスが発生しやすくなりますが、同じファイルの直近行であれば多少重複作業があっても良いのでは。

Copy link
Member

Choose a reason for hiding this comment

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

このままでもいいですし、後で変えてもいいと思います。
VER_MAJOR, VER_MINOR の後の名前が難しいです。
VER_PATCH?, VER_REVISON (VER_BUILD?)

おっけーです。あとで変える意識があるのであれば今の時点では A, B, C, D のままで良いです。

Copy link
Member Author

Choose a reason for hiding this comment

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

VER_A 定義の上あたりに、バージョン番号を示すコメントを入れてほしいです。

4ab94de で例としてコメントを記載しました。
この書き方でどうでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

ありがとうございます!見やすいです

#define TO_STR(arg) #arg
#define MAKE_VERSION_STR(a,b,c,d, sep) TO_STR(a) sep TO_STR(b) sep TO_STR(c) sep TO_STR(d)
#define MAKE_VERSION_STR_PERIOD(a,b,c,d) MAKE_VERSION_STR(a,b,c,d, ".")
#define MAKE_VERSION_COMMA(a,b,c,d) a,b,c,d
Copy link
Member

Choose a reason for hiding this comment

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

細かい指摘になりますが , の後ろには半角スペース1個入れてほしいです。マクロの都合上入れられないという事情等あれば仕方ないですが。

Copy link
Member Author

Choose a reason for hiding this comment

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

, の後ろには半角スペース1個入れてほしいです。

147d7d2 で修正しました。

#define MAKE_VERSION_COMMA(a,b,c,d) a,b,c,d

#define PR_VER_STR MAKE_VERSION_STR_PERIOD(VER_A, VER_B, VER_C, VER_D)
#define PR_VER MAKE_VERSION_COMMA(VER_A, VER_B, VER_C, VER_D)
Copy link
Member

Choose a reason for hiding this comment

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

元の定数名を踏襲しているだけなので今突っ込むところではないかもしれませんが、
もう少し分かりやすい名前に変えたいですね。調べてみたら、この PR って PRODUCT を略してるみたいですね。

個人的には PRODUCT_VERSION_STR, PRODUCT_VERSION くらいが程よい名前かな、と思いました。

Copy link
Member Author

Choose a reason for hiding this comment

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

元の定数名を踏襲しているだけなので今突っ込むところではないかもしれませんが、

そうですね。ここで変えるとややこしいと思います。

@@ -35,8 +35,6 @@ LANGUAGE LANG_ENGLISH, SUBLANG_DEFAULT
#endif //_WIN32

#define S_COPYRIGHT "Copyright (C) 2011-2017 by Lucien & Collaborators"
#define PR_VER 2,3,2,0
#define PR_VER_STR "2.3.2.0"
#define FL_VER PR_VER
#define FL_VER_STR PR_VER_STR
Copy link
Member

Choose a reason for hiding this comment

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

こっちの FL は FILE の略みたいでした。欲を言うと FILE_VERSION, FILE_VERSION_STR あたりに変更するのが心地よい感じがしますが、この PR の範囲を超える気がするのでこれは別タイミングで良いです。一応感じたことをコメント残すだけしておきます。

Copy link
Member Author

Choose a reason for hiding this comment

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

この PR では可能な限り変更量を最小にして対応しようとしています。

Copy link
Member

Choose a reason for hiding this comment

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

OKです

@kobake
Copy link
Member

kobake commented Jun 30, 2018

ご対応ありがとうございます。以下は AppVeyor 成果物の sakura-Win32-Release.zip の中身です。問題なさそうです。

Copy link
Member

@kobake kobake left a comment

Choose a reason for hiding this comment

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

LGTM

@kobake kobake merged commit de19251 into sakura-editor:master Jun 30, 2018
@m-tmatma m-tmatma deleted the feature/refactor-version-definition branch June 30, 2018 10:45
@ds14050 ds14050 added the refactoring リファクタリング 【ChangeLog除外】 label Sep 18, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…ersion-definition

バージョン定義を一箇所で定義する
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring リファクタリング 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants