isearch拡張機能のadviceによる再実装 #30
Conversation
|
あれ、これだと正規表現サーチ( |
tooro88
force-pushed
the
isearch-advice
branch
from
f2c9ab5 to
8614097
Compare
|
修正しました。isearch.el にある、isearch を開始するコマンド一通り、期待した挙動になることを確認するテストを追加しました。 |
|
全体的な構成として, もうしばらくはtc-is22とtc-ishelperを共存させて選択できるのがいいかと思います. 将来的にはあまり筋のよくない, is22の方は消していきたいですね. この場合, コード部分のcommitを以下のように分割できると, 新規部分にレビュー意識を集中できて助かります.
こうすると, それぞれのcommitの目的がはっきりして読みやすくなるかと思います. cosmeticな変更なので, わたしの方でやってもいいのですが…来週まではまとまった時間がとりづらいので, 時間がかかるかもしれません. お手間ではありますが, やっていただいた方が早いかもです… |
|
もう一つ, 現時点で気になったのは":advice"の":"です. ":"で始まるシンボルはkeyword symbolであって, ":hoge 'fuga"という感じで, キーワード引数として後ろに続くものがあるという印象です. ここは"'advice"のように"'"(single quote)のシンボルがいいかと思います. |
|
レビューありがとうございます。commit 分割はこちらでやります。今後も全修正こちらでやりますので、気兼ねなく「ああして、こうして」と言ってください。 tc-isbushumaze.el はコピーかはい、各関数は完全なコピーです。このPRの最初のコメントでも少し触れてあります。 tc-is22.el はしばらく共存そうですよね。すると、tc-is22.el 内の目に見える問題は直してしまう方がよいと思います。 commit 分割目的ごとに分割されている方が読みやすいとのことであれば、advice 化、部首/交ぜ書きサポート、wrapped search はそれぞれ独立した修正なので、わける方が各 commit がすっきりすると思います。次のようになるでしょうか。
いったんこれで作業を始めますが、「言った通りやってほしい」ということであればお知らせください。
|
よいと思います. 細かい方が目的がはっきりしてレビューの方向が定まりますし, ここまでOKという区切りもつけやすくて助かります. |
tooro88
force-pushed
the
isearch-advice
branch
from
8614097 to
31fafbb
Compare
|
commit を作り直しました。よく考えたら、「advice による isearch 実装」と「部首/交ぜ書き変換」は分けられないのでした。その2つはまとめてあります。次のようになりました。
|
|
commit 68fbc1f のコード内にコメントをいただいたので返信しました。 github の使い方がよくわからないのですが、こういう commit 内コメントがあったときって「メール通知を受け取る設定の人が」「メールを見逃さなかったときだけ」気付ける仕組みなのでしょうか? このレポジトリに関して、またはこのPRに関してついたコメント一覧のようなものを見る仕組みってありますか? |
この件は「Review」という機能を使ってコメントを書くと、このスレッドに反映されるということがわかったので、ひとまず解決となりました。 |
tooro88
force-pushed
the
isearch-advice
branch
from
31fafbb to
9c22d49
Compare
|
ブランチを更新しました。次の修正をしました。
commit群は以下のようになりました。
|
tc-setup.el
Outdated
| (cond ((memq tcode-use-isearch '(t overwrite)) | ||
| (require 'tc-sysdep) | ||
| (require tcode-isearch-overwrite-module)) | ||
| (tcode-use-isearch |
There was a problem hiding this comment.
ここはcatch allでなくて、明示的に advice にマッチする方がいいように思います。まだデフォルトを`advice'にするわけではないので。
なににもマッチしない場合は、"(warn ...)"にする and/or 保守的にはoverwrite 扱いするのがいいように思います。
There was a problem hiding this comment.
ここは im が増えたときにコードを足さないで済むよう、im、advice の方を catch all にしてたのでした。「ユーザーが変な値を指定した場合の挙動は不定」でよいかと考えていました。(advice の挙動になるというわけでもないです。advice-add が呼ばれないので。)
ミスしたときに原因がわかりにくいのも良くないですね。警告を出して overwrite にフォールバックするようにします。
There was a problem hiding this comment.
あ、tcode--ensure-valid-value()の部分でsetしてるのですね…なるほど
tooro88
force-pushed
the
isearch-advice
branch
from
9c22d49 to
44eb884
Compare
|
レビュー結果を反映しました。テストのコードも少し整理と機能追加しています。 変更した commit は以下の通りです。
|
tc-setup.el
Outdated
| (cond ((memq tcode-use-isearch '(t overwrite)) | ||
| (require 'tc-sysdep) | ||
| (require tcode-isearch-overwrite-module)) | ||
| (tcode-use-isearch |
tc-setup.el
Outdated
| (cond ((memq tcode-use-isearch '(t overwrite)) | ||
| (require 'tc-sysdep) | ||
| (require tcode-isearch-overwrite-module)) | ||
| (tcode-use-isearch |
There was a problem hiding this comment.
あ、tcode--ensure-valid-value()の部分でsetしてるのですね…なるほど
tc-setup.el
Outdated
| (tcode-use-isearch | ||
| (require 'tc-ishelper)))) | ||
|
|
||
| (defun tcode--ensure-valid-value (var vals fallback) |
There was a problem hiding this comment.
この関数は後で使われたりしますか? 関数内で変数がsetされる副作用があって、効果が関数型的でない点、condでの場合分けでも十分に書ける点を考えると、他での用途がなければ、condでそのまま書きたいかと思います。
There was a problem hiding this comment.
tcode--ensure-valid-value は、現状予定はないものの、将来別の変数で使うことも想定して書いたものです。tcode-use-isearch 一つでの使用に限ったとしても、論理的には複数個所から使うかも知れない処理(tcode-use-isearch の最初の使用の前に置きたい。たまたま最初の使用場所がこの一箇所に限られるだけ)なので、関数になっている方が自然だと感じます。
(仮に論理的にも1ヶ所でしか使われない関数だとしても、独立した意味のあるまとまりとして名前の付けられる操作は、どんどん関数にすべき、それが良いプログラムであると私は考えます。)
副作用が見えにくいことを問題視しているのであれば、こう考えてもらうことはできないでしょうか? もともと tcode-use-isearch は nil t overwrite advice の4値しか取らない変数で、そうでない値が来たときはバグです。そのようなバグが無いことを保証するのが tcode--ensure-valid-value です。assert 文のようなものです。もし assert 文のように、tcode--ensure-valid-value の中で、警告の代わりにエラーにしていたら、問題視しないのではないでしょうか。だとしたら、現行版はエラー版でやりたいことと同等で、より親切な挙動になっていると考えられないでしょうか。
There was a problem hiding this comment.
ここはかなり悩ましいのですが、 assert が成り立つのは、それが well-known で、名前から「条件が満たされなければ止まる・以降は前提が成立している」と読み手が期待できるからだと思います。今回の関数は新規追加で、その共有された期待がありません。
おそらく ensure という名前もその一因だと思います。これだけでは error なのか fallback なのか、あるいは set まで行うのかが分からず、実装を見るまで副作用(第三引数が fallback であって、それを set すること) が読み取れません。関数化は、ある点、可読性のためのものですが、この点ではむしろ推測コストを上げていると感じます。
もし関数化するのであれば、名前に set や fixup、fallback などを入れて、副作用が明示的であってほしいです。
個人的には、この種の処理は初期化時に一度だけ行えば十分で、副作用のある処理を使いやすくする必要はなく、下の cond で setq する形で足りると思っています。(結局、 cond で、この関数内の member 的なことをするわけなので)
There was a problem hiding this comment.
大変失礼ながら、このお返事にはすぐには納得のできない部分があって、議論にお付き合いいただけるのなら議論を続けたいという気持ちがあります。ですが、それはひとまず置いておいて、副作用が問題ということでしたら、次のコードはどうでしょうか。fallback 値への変更をなくしました。これならシンプルで、私にとっては、妥協策としてでなく、単体で満足の行くコードです。
(defun tcode--load-isearch ()
"`tcode-use-isearch' の設定に応じて必要なファイルをロードする。"
(cond ((eq tcode-use-isearch 'overwrite)
(require 'tc-sysdep)
(require tcode-isearch-overwrite-module))
((eq tcode-use-isearch 'advice)
(require 'tc-ishelper))
((eq tcode-use-isearch nil) ) ; do nothing
(t
(lwarn 'tcode :warning "Invalid value `%s' for `tcode-use-isearch'."
tcode-use-isearch))))ひとまず、以下の部分は置いておいて、このコードで ok かだけお知らせいただけないでしょうか。
以下はいただいたお返事に対する私の主張です。もし上のコードで問題無い(または微修整だけで済む)なら、以下をお読みいただく優先度は低いです。今後もこういう観点で意見がわかれるかも、ぐらいの参考情報です。コメントいただけたらありがたいですし、「へえー」と言って閉じていただいても構いません。
ここはかなり悩ましいのですが、
assertが成り立つのは、それが well-known で、名前から「条件が満たされなければ止まる・以降は前提が成立している」と読み手が期待できるからだと思います。今回の関数は新規追加で、その共有された期待がありません。
新規コードに共有された知識が無いのは当然なので、この話はよく分かりません。組み込み関数以外でも、エラー終了や変数の書き換えを目的とする関数をどんどん新規に作ってよいと私は考えます。
おそらく
ensureという名前もその一因だと思います。これだけでは error なのか fallback なのか、あるいは set まで行うのかが分からず、実装を見るまで副作用(第三引数が fallback であって、それを set すること) が読み取れません。関数化は、ある点、可読性のためのものですが、この点ではむしろ推測コストを上げていると感じます。
コードを読んでいて、知らない関数が現れたら、ドキュメントや本体を読むべきだと私は考えます。「推測コスト」とありますが、推測で済ませてはいけないと考えます。
ドキュメントや本体を読んだ上で、名前が misleading だという指摘はあり得ると思います。tcode--ensure-valid-value が misleading だとは思っていませんでした。もちろん、もっといい名前を提示されたら、「そっちの方が全然いいじゃん」となった可能性は大いにあったと思います。
もし関数化するのであれば、名前に set や fixup、fallback などを入れて、副作用が明示的であってほしいです。
もう tcode--ensure-valid-value は無しの案を出したのでこれ以上この関数を擁護しても意味が無いのですが、これについては、副作用の何を問題視しているのかがよくわかりません。
一般論として、変数の値がこっそり書き換わるのはよくないと思います。ですが、それは valid な値が valid な値に変わる場合ではないでしょうか。「invalid な値であることを仮定してコードを書いていたが、valid な値に変わっていて困った」とはならないはずです。(しかも、関数の先頭に書かれている処理です。) さらに、前にも書いたように、値を書き換えることが tcode--ensure-valid-value の目的ではありません。tcode-use-isearch が nil overwrite advice 以外の値でないことを保証すること(C の enum のようなアイデアを、lisp で表現すること)がこの関数の目的であって、それをエラー終了によって実現するのか、fallback 値への書き換えによって実現するのかは「実装の詳細」と言っていいぐらいです(ドキュメントに FALLBACK 値をセットすると書いている以上「実装の詳細」は言い過ぎですが、もっとぼかしたドキュメントになっていて、「FALLBACK 引数は現在のところ使われていない」となっていても用途が達成できるぐらいには詳細です。)
別の観点として、tcode--ensure-valid-value が副作用をおこすのがまずくて、tcode--load-isearch で副作用をおこしてもよいのはなぜか。この質問の答えが浮かばないぐらい、私は副作用のまずさがわかっていません。
個人的には、この種の処理は初期化時に一度だけ行えば十分で、副作用のある処理を使いやすくする必要はなく、下の
condでsetqする形で足りると思っています。(結局、condで、この関数内のmember的なことをするわけなので)
「副作用のある処理を使いやすくする必要はなく」の部分は、よくわかりません。次の疑問が浮かびます。
-
繰り返しですが副作用のどんな点を問題視しているのかがよくわかりません。emacs lisp は基本的には副作用で処理を進めていく言語ではないでしょうか。今後のレビュー対象も、副作用だらけです。もし今後、副作用に関する問題があったときは、どういう点を問題視しているのか少し詳しめに教えてほしいです。
-
「使いやすく」は、関数化することを指しているのでしょうか。確かに、処理を1回書くだけで何度も使えて便利、というのは関数の利点の一つです。しかし、私は、その利点のために関数化がしたかったわけではありません。私は「作者のアイデアがよく表現されているプログラムがよいプログラムである」と教わり、自分でもそれは正しいと思っています。関数は、その方針でプログラムを書くための基本的な道具です。1つの関数を複数個所から呼ぶ例で言うと、単に同じことを何度も書く手間が省けていること以上に、これら複数個所でやっている処理が「まさに同じ処理である」ということが表現されていることが重要なメリットであると考えます。今回の例では(小さいコードなのでありがたみが薄いですが、もっと複雑なコードに敷衍して考えてほしいです)、
tcode--ensure-valid-valueを関数化することによって、tcode--load-isearchの残りの部分とは独立した処理であることが表現されます。利点として例えば、tcode--ensure-valid-valueに入った修正を確認するときに、tcode--load-isearchについての詳しい知識がいらないことがはっきりしています。一つの関数内に書き下してあったらそうはいきません。というわけで、私は関数化を「良いこと」ととらえているので、それを止めるように言われて驚いている次第です。(もちろん、関数化を上手くやれないないので戻した方がマシという指摘はあると思います。今回挙げていただいた理由では、上手くやれていないとは思えませんでした。)
There was a problem hiding this comment.
大変失礼ながら、このお返事にはすぐには納得のできない部分があって、議論にお付き合いいただけるのなら議論を続けたいという気持ちがあります。ですが、それはひとまず置いておいて、副作用が問題ということでしたら、次のコードはどうでしょうか。fallback 値への変更をなくしました。これならシンプルで、私にとっては、妥協策としてでなく、単体で満足の行くコードです。
(defun tcode--load-isearch () "`tcode-use-isearch' の設定に応じて必要なファイルをロードする。" (cond ((eq tcode-use-isearch 'overwrite) (require 'tc-sysdep) (require tcode-isearch-overwrite-module)) ((eq tcode-use-isearch 'advice) (require 'tc-ishelper)) ((eq tcode-use-isearch nil) ) ; do nothing (t (lwarn 'tcode :warning "Invalid value `%s' for `tcode-use-isearch'." tcode-use-isearch))))ひとまず、以下の部分は置いておいて、このコードで ok かだけお知らせいただけないでしょうか。
これはOKだと思います. やるなら, ここで overwriteのbranchを後に持ってきて
(cond
...
(t
(unless (eq tcode-use-isearch 'overwrite)
(lwarn ...)
(setq tcode-use-isearch 'overwrite))
(require 'tc-sysdep)
(require tcode-isearch-overwrite-module)))と (advice など以外の) non-nil value を fallback してもいいと思います.
There was a problem hiding this comment.
前提として、関数化や副作用そのものに反対しているわけではありません. 今回の懸念は、「任意のグローバル変数を書き換えうる汎用関数」を追加することによる、
変更地点の不可視化とメンテナンス性の低下です. この部分, 副作用という言い方が曖昧であったかと思います. 申し訳ないです.
一般的な話として, もちろんコードの意味のあるまとまりを関数にするのはいいことです. しかし, そこには「なにを」「どのように」関数としてまとめるのかという評価も伴います.
まず, 「なにを」の視点からいうと, わたしは, グローバルな変数を非明示的に書きかえるルートを追加する関数について, 懸念を持ちます. これは変数がどこで書きかえられているのかを, 見にくくし, メンテナンス性を下げると思うからです.
その点, 上記コメントのように明示的に (setq tcode-use-isearch ...) するのは問題ないと思います. なぜなら, ここでは明らかに tcode-use-isearch がこの場で書きかえられているのだということがわかります. 極論, ざっくりと grep "setq tcode-use-isearch" とすれば, 全ての変更地点を見つけることができるでしょう.
一方, 関数が受け取ったシンボルに set する場合, そこで何が書きかえられるのかは, 全ての呼び出しをチェックする必要があります. 今回はまだ1つの関数ですが, 複数の set する関数があったらどうでしょうか? それぞれの関数の呼び出しをチェックしていくことになります. そういったメンテナンス性の低下をおそれて, こういう関数の追加には懐疑的です.
もちろん, 今回の関数も上手に扱えば便利なものだとは思います. しかし, 便利であるがゆえに, 将来意図せず利用箇所が増え, 結果として変数の変更地点が見えにくくなることを危惧しています.
以下はいただいたお返事に対する私の主張です。もし上のコードで問題無い(または微修整だけで済む)なら、以下をお読みいただく優先度は低いです。今後もこういう観点で意見がわかれるかも、ぐらいの参考情報です。コメントいただけたらありがたいですし、「へえー」と言って閉じていただいても構いません。
ここはかなり悩ましいのですが、
assertが成り立つのは、それが well-known で、名前から「条件が満たされなければ止まる・以降は前提が成立している」と読み手が期待できるからだと思います。今回の関数は新規追加で、その共有された期待がありません。新規コードに共有された知識が無いのは当然なので、この話はよく分かりません。組み込み関数以外でも、エラー終了や変数の書き換えを目的とする関数をどんどん新規に作ってよいと私は考えます。
おそらく
ensureという名前もその一因だと思います。これだけでは error なのか fallback なのか、あるいは set まで行うのかが分からず、実装を見るまで副作用(第三引数が fallback であって、それを set すること) が読み取れません。関数化は、ある点、可読性のためのものですが、この点ではむしろ推測コストを上げていると感じます。コードを読んでいて、知らない関数が現れたら、ドキュメントや本体を読むべきだと私は考えます。「推測コスト」とありますが、推測で済ませてはいけないと考えます。
ここは「どのように」に関わる問題かと思います. もちろん, 知らない関数についてドキュメントや本体を読むべきであるのは正当です.
しかしながら, このコードはボランティアベースのOSSです. できるだけカジュアルにぱっと読んで大枠が分かることも大事にしたいです. もちろんコードを変更する人はどうせ中身まで読まなければならないとしても, コードをまずは読みたい人が, 簡単に読めることもOSSでは大切だと思っています. なので小さいコードであればあるほど, well-known であったり, 期待が明白であってほしいです. ここはある種の思想かもしれませんが, 重要な設計判断だと考えています.
もし関数化するのであれば、名前に set や fixup、fallback などを入れて、副作用が明示的であってほしいです。
もう
tcode--ensure-valid-valueは無しの案を出したのでこれ以上この関数を擁護しても意味が無いのですが、これについては、副作用の何を問題視しているのかがよくわかりません。一般論として、変数の値がこっそり書き換わるのはよくないと思います。ですが、それは valid な値が valid な値に変わる場合ではないでしょうか。「invalid な値であることを仮定してコードを書いていたが、valid な値に変わっていて困った」とはならないはずです。(しかも、関数の先頭に書かれている処理です。) さらに、前にも書いたように、値を書き換えることが
tcode--ensure-valid-valueの目的ではありません。tcode-use-isearchがniloverwriteadvice以外の値でないことを保証すること(C の enum のようなアイデアを、lisp で表現すること)がこの関数の目的であって、それをエラー終了によって実現するのか、fallback 値への書き換えによって実現するのかは「実装の詳細」と言っていいぐらいです(ドキュメントに FALLBACK 値をセットすると書いている以上「実装の詳細」は言い過ぎですが、もっとぼかしたドキュメントになっていて、「FALLBACK 引数は現在のところ使われていない」となっていても用途が達成できるぐらいには詳細です。)別の観点として、
tcode--ensure-valid-valueが副作用をおこすのがまずくて、tcode--load-isearchで副作用をおこしてもよいのはなぜか。この質問の答えが浮かばないぐらい、私は副作用のまずさがわかっていません。
この部分は, わたしの「副作用」という言い方がよくなかったと思います. 上記のとおり, 任意の変数をとり, それを変更しうるのが「便利すぎる」ことに懸念があります.
この点でいうと, もしこの関数が以下のように tcode-use-isearch 専用であれば問題を感じないです.
(defun tcode--ensure-use-isearch-value ()
"..."
(unless (memq tcode-use-isearch '(overwrite advice nil))
(lwarn 'tcode :warning
"Invalid value `%s' for `tcode-use-isearch'; falling back to `overwrite'."
tcode-use-isearch)
(setq tcode-use-isearch 'overwrite)))これは, ここで tcode-use-isearch を変更していることが, 関数の呼び出し元を見なくても明白だからです. また valid な value がこの関数内に閉じているのも, 良いと感じています. 複数の呼び出しがあって, valid value がそれぞれ違う(変更し忘れなどで)ことを防げます.
* 「使いやすく」は、関数化することを指しているのでしょうか。確かに、処理を1回書くだけで何度も使えて便利、というのは関数の利点の一つです。しかし、私は、その利点のために関数化がしたかったわけではありません。私は「作者のアイデアがよく表現されているプログラムがよいプログラムである」と教わり、自分でもそれは正しいと思っています。関数は、その方針でプログラムを書くための基本的な道具です。1つの関数を複数個所から呼ぶ例で言うと、単に同じことを何度も書く手間が省けていること以上に、これら複数個所でやっている処理が「まさに同じ処理である」ということが表現されていることが重要なメリットであると考えます。今回の例では(小さいコードなのでありがたみが薄いですが、もっと複雑なコードに敷衍して考えてほしいです)、`tcode--ensure-valid-value` を関数化することによって、`tcode--load-isearch` の残りの部分とは独立した処理であることが表現されます。利点として例えば、`tcode--ensure-valid-value` に入った修正を確認するときに、`tcode--load-isearch` についての詳しい知識がいらないことがはっきりしています。一つの関数内に書き下してあったらそうはいきません。 というわけで、私は関数化を「良いこと」ととらえているので、それを止めるように言われて驚いている次第です。(もちろん、関数化を上手くやれないないので戻した方がマシという指摘はあると思います。今回挙げていただいた理由では、上手くやれていないとは思えませんでした。)
コードはコードとして残るので, 意図と離れた使われ方をすることがあります. 今回, 意味のまとまりのため(だけ)に関数として作ったとしても, それは他から使いやすく使われていくことを妨げることはできません. set している場所から, なにが変更されているのか見えにくい, 変数名でgrepしてもすぐにはそれが setq とは見えにくい…というのをわたしはやはり危惧しています. これが関数として, より大きなコードであり, 複雑な処理をして, 呼び出し側で処理しにくいのであれば…まだわかるのですが, 今回のようにコードが小さく, 代替手段も明確に存在する局面では, 関数化による表現上の利点よりも、将来のメンテナンスコストの方を重く考えてしまいます.
There was a problem hiding this comment.
議論にお付き合いいただきありがとうございます。納得しました。
まず、コードは fallback 値への書き替えの無いバージョンの commit をもう作ってしまったので、このままでお願いします。(ユーザーの設定ミスはすぐに直してもらった方がよいと考えると、こちらの方がよい気もして来ました。)
set の使用が問題である(標語として「grep しにくくなる」)という観点は非常に明快で、私はその観点がすっぽり抜け落ちていました。言われて見ると積極的に賛同したい考え方です。(そういえば、「オブジェクト指向って grep で追跡しにくくていやだなあ」と以前考えていたこともあったのを思い出しました。)
挙げていただいた tcode--ensure-use-isearch-value のような関数なら問題無いというのも朗報で、それなら(私から見ると)気にすべき意見の相異は無いと言えそうです。
また valid な value がこの関数内に閉じているのも, 良いと感じています. 複数の呼び出しがあって, valid value がそれぞれ違う(変更し忘れなどで)ことを防げます.
これはおっしゃる通りで、私が主張していた路線に沿うなら、むしろ私が率先してこう書いているべき(tcode--ensure-valid-value よりもまず先に tcode--ensure-use-isearch-value があるべき)でした。なるほど。
ほかの点も同意です。返信にずいぶんお手間をとらせてしまったと思いますが、おかげで納得のできる結論となりました。ありがとうございました。
|
emacs-24、25 をサポート外にしたのはよいのですが、今の修正方法だとそれらを起動して tc-setup を読み込んだだけでエラーになってしまいます(存在しないマクロ isearch-define-mode-toggle を使用している)。それはちょっと不便に感じたので、エラーにはならないように直します。 |
`isearch-toggle-tcode` is only referenced in obsolete XEmacs-specific code. Remove both.
- Clarify the variable's docstring to better describe its behavior. - Remove the redundant value t. - For values 0 and 1, fix incomplete handling of input method state changes: update the mode line and maintain consistent internal state by using `isearch-toggle-input-method`.
The term "wrapped search" in tc-is22.el is confusing. It refers to a search that ignores line wrapping, while Isearch already uses "wrap" to describe restarting a search from the opposite end of the buffer. Rename "wrapped search" in tc-is22.el to "line-fold search" to avoid this ambiguity.
Split tc-is22.el into two files for future reuse: - Keep in tc-is22.el only the functions that override Emacs internal functions. These will be reimplemented later. - Move the remaining functions to tc-iscommon.el, which will be reused in the forthcoming reimplementation.
Upcoming changes will provide multiple implementations of the Isearch extensions. The variable `tcode-use-isearch` will be used to select among them. Rename the default value `t` to `'overwrite`, which more accurately describes the default implementation.
Rename the variable `tcode-isearch-type` to `tcode-isearch-overwrite-module`, which more accurately describes its role.
tooro88
force-pushed
the
isearch-advice
branch
from
44eb884 to
bb33e98
Compare
|
レビュー結果を反映しました。以下が内容に変更のある commit です。
|
naota
left a comment
naota
left a comment
There was a problem hiding this comment.
テスト部分まで全部見ようかと思ったのですが…今日はここまでですみません。明日また見ます。
test/tctest-env.el
Outdated
|
|
||
| (defun tctest-env-main () | ||
| (let* ((keywords-packed (or (getenv "TCTEST_ENV") | ||
| (error "TCTEST_ENV not specified"))) |
ごゆっくりで全然構いません。私の方には全く急ぐ理由がなくて、見てもらえているだけでありがたいです。 よく考えると、テストの部分に直しがあってもそれ以前の commit に影響があるとは考えにくいので、別 PR にした方が良い気がしてきましたが、どうでしょうか。自分で一緒にしておいてなんですが。 |
基本的には変更とテストは同時に入るべきですね。ただ今回、テストの基本的な機能自体の実装なので別PRでもよいといえばよいですね。 まあ分けた方が若干このPRのマージは早くなるかとは思いますが…もう見始めてはいるのであまり変わりはしないかも… |
なるほど。ではこのままで行きましょう。 |
Add an `'advice` option to `tcode-use-isearch`. When this option is selected, the loader skips tc-is22.el and instead loads tc-ishelper.el, which will reimplement the features of tc-is22.el using advice in forthcoming commits.
The Isearch extensions provided by tc-is22.el override several internal Emacs functions. This approach is error-prone and can miss new features introduced in recent Emacs versions. tc-is22.el provides two features: 1. line-fold search, which will be reimplemented in the next commit, and 2. user input hooking, which is reimplemented here. Replace the modification of `isearch-printing-char` with advice that achieves the same behavior -- invoking the T-Code input method when necessary -- so that the existing bushu/mazegaki conversion code in tc-iscommon.el continues to work unchanged.
tc-is22.el implements line-fold search by modifying internal Isearch functions to convert a search string into a regexp. Since Emacs 25, `isearch-regexp-function` has provided a cleaner way to perform such conversions. Use `isearch-regexp-function` to reimplement line-fold search, and enable this implementation when `tcode-use-isearch` is `'advice`. Together with the change in the previous commit, this completes the reimplementation of the features previously provided by tc-is22.el.
tooro88
force-pushed
the
isearch-advice
branch
from
bb33e98 to
c85b5fa
Compare
|
レビュー結果を反映しました。変更のあったcommit:
|
|
関数名、変数名の 当初、tctest-play.el で定義する名前について、「内部関数は いっそ内部外部の区別をやめて、全部 ok なら、レビューのキリのいいタイミング(こちらが修正するターンになったとき)にまとめて変更します。または、「区別する路線を徹底するべき」「ok だがこの PR の後で」などご意見あればお知らせください。 |
年末年始なかなかreviewできずすみません… どうせemacsでは内外を厳密に区別することもできないですし, package全体としても特別の区別も現状できていませんし, この方針で問題ないかと思います. |
|
助かります。次回何かを修正するタイミングで一緒に直します。 |
naota
left a comment
naota
left a comment
There was a problem hiding this comment.
いくつかコメントを書きましたが, 基本的にはoptionalです. 全体的にはもう完成でマージできるものと思います.
上の tctest-- の変更をいれていただいて, あとはマージでよさそうと思っています.
せっかくなので、コメントいただいた分も反映したいと思います。 ついでに次の2点も直させてください。どちらも一行程度の修正です。
|
New files: - tctest.el : Test items. - tctest-play.el : Test driver. Simulate user inputs using keyboard macro. - tctest-env.el, run.bash : Convenient scripts for selecting isearch options.
Add `:quiet` option to tctest-play and enable it when `quiet` keyword is specified for tctest-env.
tooro88
force-pushed
the
isearch-advice
branch
from
c85b5fa to
87bd037
Compare
|
レビュー結果を反映しました。追加で、 変更したcommit:
|
|
マージしました! 長きにわたりありがとうございました. |
2 participants
「emacs 本体の関数の上書きを無くしたい #29」に書いた、advice による isearch 拡張と wrapped-search の実装です。テストも付けました。input method 系の実装は、この commit に追加する形で後日、別 PR にします。
以下、commit log に書ききれなかったコメントです。
Reimplement isearch extensions using advice (commit e8391d7)
isearch-define-mode-toggleというマクロを使いました。これには、「キーバインドを作る」という望んでない機能もついていて、オフにできないのですが、まあいっかと思ってそのままにしてあります。iseach 中にM-s @で wrapped search がオンオフできます。tcode-isearch-start-stateについては、tcode-isearch-start-state を 0 か 1 にすると、[TC]表示が連動しない。 #28 の問題を修正した版を入れました。この機能の発案者がどういう意図だったのか(「バッファと独立」とは?)、元の実装がその意図に沿ったものだったのか、いまいちよくわからないのですが、元の実装の挙動を仕様と考えて、DOCSTRING を明確化しました。tは無くしてあります。Add a section about isearch extensions to README.md (commit b9a73bc)
Add ert tests for isearch extensions (commit f2c9ab5)
:expected-resultが:fail) としてあるので、テスト全体では成功扱いになるはずです。(赤いFマークが出ない)。(setq tcode-use-isearch nil)(setq tcode-use-isearch t)(setq tcode-use-isearch :advice)以上です。修正点、疑問点等あればお知らせください。些細な問題でも教えていただけるとありがたいです。