[WIP] 冗長な空行を削除 Part2

[m-tmatma]

冗長な空行を削除 Part2
#719 で対応できないものを対応する。

[m-tmatma]

以下のようなコードがあったときに 266行目の改行を取り除く。

sakura/sakura_core/_main/CCommandLine.cpp

Lines 262 to 268 in aab76a5

	// 2007.09.09 genta オプション判定ルール変更．オプション解析停止と""で囲まれたオプションを考慮
	if( ( bParseOptDisabled ||
	! (pszToken[0] == '-' || pszToken[0] == '"' && pszToken[1] == '-' ) )){
	if( pszToken[0] == _T('\"') ){
	CNativeT cmWork;

PR 後はこんなコードになる。

①

		//	2007.09.09 genta オプション判定ルール変更．オプション解析停止と""で囲まれたオプションを考慮
		if( ( bParseOptDisabled ||
			! (pszToken[0] == '-' || pszToken[0] == '"' && pszToken[1] == '-' ) )){
			if( pszToken[0] == _T('\"') ){

末尾の { が独立した行にあると以下のようになる。

②

		//	2007.09.09 genta オプション判定ルール変更．オプション解析停止と""で囲まれたオプションを考慮
		if( ( bParseOptDisabled ||
			! (pszToken[0] == '-' || pszToken[0] == '"' && pszToken[1] == '-' ) ))
		{
			if( pszToken[0] == _T('\"') ){

コメント

現状のまま、空行を取り除くとぱっと見た目で、制御構造が見にくいかもしれない。

どう思いますか?

1. 現状のままがいい (PR 前)
2. この PR 適用後 ①
3. { を独立させる。②

私の好み的には { を独立した行に置きたいですが、

[takke]

私は 1. 現状のまま が好みです。
Windows 用のコードだと 3. もよく見かけるので慣れてきましたが 2. は分かりにくくなりそうな印象です。

[m-tmatma]

この PR は CodeFactor の警告を減らすのが目的です。

[ds14050]

なぜ CodeFactor を黙らせる方向で修正しないのですか？

読みやすさは主観です。機械に判断させ機械で修正するならただ一人の主観さえも存在しなくなります。読みにくくなるようにコードを書いた人はいないでしょう。機械にお伺いを立てるのは自分で書くコードに限定すると波風が立たないと思います。

[berryzplus]

私は 1. 現状のまま が好みです。
Windows 用のコードだと 3. もよく見かけるので慣れてきましたが 2. は分かりにくくなりそうな印象です。

便乗。現状のままでよいと思います。
2は読みづらいです。行間多すぎコードの行を詰めたいときはコメント入れてます・・・。
3は許容しますし、たまにやります。なんか古めかしいイメージがあるので統一は尻込み。

どちらかというと if の後に空白がないことが気になります。
そして丸括弧が１個余分に多い・・・コンパイラ依存の挙動への対策かな？
演算子の結合順序が「常識」と違うコンパイラもあるらしいので 😄

[beru]

人工知能によるチェックを回避しつつ可読性を極力排す事でメンテナンスしにくくして属人化する技術が必要になる未来が来そうですね。いたちごっこかもしれませんが。

[m-tmatma]

この PR のうちスクリプトのバグ修正のみを #725 で投げました。

[m-tmatma]

なぜ CodeFactor を黙らせる方向で修正しないのですか？

修正するたびに黙らせるのが手間なので、簡単に修正できるものはしゅうせいしちゃいたいということです。

ただこの PR は 適用は難しいかなと思っています。

[ds14050]

修正するたびに黙らせるのが手間なので、簡単に修正できるものはしゅうせいしちゃいたいということです。

個別に ON/OFF セッティングできるはずだと思ったので調べました。

2. Click Ignore Issues like this to exclude selected issue pattern for entire repository.

こういう対応を選ばない理由についての疑問でした。

ちなみに自分は Ignore を選ぶことすらせずに文字通り Ignore する腹づもりでした。

[berryzplus]

ちなみに自分は Ignore を選ぶことすらせずに文字通り Ignore する腹づもりでした。

ツボった・・・ｗ

誰かがignoreポチらないとレビューに進めない仕組みになってそうです。
気にせず ignore で行こうかと思ってましたが、 #631 で気になって直しちゃいました。

空白削除はコンフリクト発生率が高いので、やるなら早めに対処がよいと思います。

[beru]

誰かがignoreポチらないとレビューに進めない仕組みになってそうです。
気にせず ignore で行こうかと思ってましたが、 #631 で気になって直しちゃいました。

え、そうなんですか？そんな事は無い（つまり CodeFactor の指摘は無視しても問題無い）と思っていたんですが…。

[berryzplus]

誰かがignoreポチらないとレビューに進めない仕組みになってそうです。
気にせず ignore で行こうかと思ってましたが、 #631 で気になって直しちゃいました。

え、そうなんですか？そんな事は無い（つまり CodeFactor の指摘は無視しても問題無い）と思っていたんですが…。

正確には「レビューに進めない」じゃなくて、
「approve貰ってもマージできない」になると思っています。
そう思ってる根拠は、CodeFactorでissueがあると
appveyorのビルド失敗と同じように「×」が付くから・・・。

あれ？appveyorビルド通らなくてもマージできましたっけ？
そうだとするとignoreボタン押さず、ignoreするのも可能なのかな。

[beru]

正確には「レビューに進めない」じゃなくて、
「approve貰ってもマージできない」になると思っています。
そう思ってる根拠は、CodeFactorでissueがあると
appveyorのビルド失敗と同じように「×」が付くから・・・。

あれ？appveyorビルド通らなくてもマージできましたっけ？
そうだとするとignoreボタン押さず、ignoreするのも可能なのかな。

マージは出来るという認識です。そうじゃないと CodeFactor の指摘に振り回されちゃうので好ましくないと思います。

[ds14050]

設定で check が通ってないとマージできないようにはできると思いますが、サクラエディタではその設定をしていないはずです。この文章を読む限りそうではないはずです。

Merging is blocked
Merging can be performed automatically with 1 approving review.

[beru]

設定で check が通ってないとマージできないようにはできると思いますが、サクラエディタではその設定をしていないはずです。この文章を読む限りそうではないはずです。

Merging is blocked
Merging can be performed automatically with 1 approving review.

ですよね。つまり [WIP] が付いていようが無かろうが存在する PR を片っ端から全て Approve して Merge しまくってもシステム的に問題ないはず。いや私が管理側に排除されてしまうかもしれませんが…。

[m-tmatma]

sakura/sakura_core/_main/CCommandLine.cpp

Lines 262 to 268 in aab76a5

	// 2007.09.09 genta オプション判定ルール変更．オプション解析停止と""で囲まれたオプションを考慮
	if( ( bParseOptDisabled ||
	! (pszToken[0] == '-' || pszToken[0] == '"' && pszToken[1] == '-' ) )){
	if( pszToken[0] == _T('\"') ){
	CNativeT cmWork;

というような記述で

PEP8 のように if 文の続きと
本文の部分でインデントを変えれば見やすくなる。

# 引数とそれ以外を区別するため、スペースを4つ(インデントをさらに)加える
def long_function_name(
        var_one, var_two, var_three,
        var_four):
    print(var_one)

[AppVeyorBot]

✅ Build sakura 1.0.2040 completed (commit 09c90a40d7 by @m-tmatma)

[m-tmatma]

コンフリクトしていたので解消した。
でも#959 を投げたので閉じる