Closed Shick1112 closed 7 months ago
@Shick1112 ありがとうございます。まさか逆の処理を作る方が現れるとは思ってもいませんでした。大変便利な機能なので、取り込むことに全く異論はありません。
全てのタグを追加したプルリク #102 を確認させていただきましたが、そちらをmasterに取り込むための作業を先にした上で、再度動作確認等させてください。おそらくですが、Text2Frameのテストケースの逆版のテストケースをスクリプト等で作り直して、動作確認することになると思います。
いずれにしても、Frame2Textにつきましては、 #102 の完了をお待ちください。お時間とってしまい恐縮ですが、よろしくお願いします。
@yktsr @Shick1112 そろそろ
の終わりが見えてきたので、Frame2Textのレビューのことも考えようと思います。
Frame2Textは、以下のようなテストでバグ・エラーがないか確かめて、リリース可能か判定したいと考えています。
./test/test_json_eq.js内の、テストファイルのインプットとアウトプットのリストであるtests変数内の要素それぞれについて以下の処理を実行する。
この処理を実行して、すべての要素についてOKがでれば、リリース可能ということにしたいと思います。(wikiやREADMEへのFrame2Textの記載も作業には含めます)
以上の前提で、お二人にお願いがあります。
まずお二人にお聞きしたいのは、この上述の手順で問題なさそうか?ということです。何か気がかりなことがありましたら、ご意見ください。
また、このテストの実装は、 @yktsr にお願いしたいです。https://github.com/yktsr/Text2Frame-MV/pull/108 が master にマージされてから、こういう仕組みづくりおよびテストコードの変更の実装は可能でしょうか? (おそらくコンフリクトはしますが、#108 が終わる前に先んじて実装していただいても構いません。)
@Shick1112 さんには、上述したテストで発見されたバグやエラーを解決してほしいです。
以上です。よろしくご検討ください。
@HidetoshiKawaguchi お疲れ様です。 Frame2Textのバグやエラーの修正了解しました。 また、Frame2Textのテストについてもそれで問題無いと思います(Frame2Text→Text2Frameの手順で一致比較)
それとFrame2TextについてはText2Frameの仕様変更についての修正をまだ取り込んでいないので、 ちゃんとしたテストが始まる前にこちらでそれらの修正を取り込んだ方がいいでしょうか? ※全てをテストが始まる前に直せるかわかりませんが、このままテストするとバグやエラーが多発すると思うので
以上です。よろしくお願いいたします。
@Shick1112 ありがとうございます。 いえ、テストが始まってから修正を始めても大丈夫です。Text2Frameの開発でも、もともとテストを書いてから実装をしていました。テストと機能実装のタイミングは、どちらが先でも大丈夫な認識です。むしろ、テストが先にあったほうが実装の指針になり、エラーやバグに即座に気付けるのでよろしいかと思います。 エラーやバグが多発して鬱陶しいかもしれませんが、その段階であればテストは実行しなくても大丈夫です。ご自身のよろしいタイミングで手動実行して、動作確認をお願いします。
検討するので、しばしお待ちを。
準備ができました。
npm run test_frame2text
const consoleStub = sinon.stub(console, 'log');
npm run lint
に統合されています。おすすめの検討方法としては、
node Frame2Text.js -m map -i ./test/expected_audio.json -o /tmp/tmp.txt -e 1 -p 1
node Text2Frame.js -m map -t /tmp/tmp.txt -o data/Map001.json -e 1 -p 1 -w true
この操作で、./test/expected_audio.json -> /tmp/tmp.txt -> data/Map001.json になります。
@HidetoshiKawaguchi f143164 は、feature/add_all_tag にもcherry-pickして、形式は以降こちらに合わせて欲しいです。
@yktsr 対応ありがとうございます。変更内容、確認しました。問題なさそうです。 cherry-pickの件も承知して、実施しておきました。feature/add_all_tagブランチは、feature/Frame2Text ブランチの最新のテストケースの形式に準じた状態になっています。
@Shick1112 現状の feature/Frame2Text ブランチには、feature/add_all_tag ブランチの最新にあるすべてのテストケースは含まれていません。しかし、feature/Frame2Text ブランチと feature/add_all_tag ブランチのテストケースの差分は、仕様変更していないタグたちについてのものです。つまり、feature/Frame2Text ブランチ内のテストケースに対応すれば、Text2Frameの仕様変更に伴って生じたFrame2Textの仕様変更にも対応することになるはずです。 なので、私としては実装についてはもう始めて頂いて問題ない認識です。お時間のあるときに、よろしくお願いします。
テストケースが揃っていないとややこしいかと思って、一応 feature/add_all_tag をMerge して揃えておきました。
@HidetoshiKawaguchi @yktsr
対応ありがとうございます! Frame2Textの実装を進めていきます。
@HidetoshiKawaguchi @yktsr お疲れ様です。
Frame2Textの全体の仕様について確認したいことがあります。 Frame2Textで出力されるタグの引数(省略できるタグ等も)は全て省略しないで出力される仕様でよろしいでしょうか?
Text2Frameで引数等を省略するとタグを記載するのが楽になるという利点がありますが、 Frame2Textの場合は勝手に出力されるので楽になるという利点が無く、 タグを見る際に省略してないほうが分かりやすいと思ったためです。
結局はText2Frameでインポートする際に同じ結果にはなるのですが、 一応仕様として抑えておいた方が良いと思いました。
以上です。よろしくお願いいたします。
@Shick1112 確認ありがとうございます。はい、省略しないで出力する仕様で良いと思います。 理由は仰る通りのことで、省略されずに出てきたほうが基本的には分かりやすいですよね。 めったに使わないオプションとかだと、省略したほうが分かりやすい場合もあるかもしれませんが、現時点でそこまで考える必要はないと思います。
よろしくお願いします。
@Shick1112 Text2Frameのwikiのfeature/add_all_tagブランチに、マスターとなる全タグの仕様を追加しました。 実装中に仕様を確認したくなった時、これが一番便利かと思います。 以下のコマンドでクローンして、中でブランチを切り替えて、「追加機能(その他イベントコマンドの組み込み).md」内にあります。
git clone https://github.com/yktsr/Text2Frame-MV.wiki.git
よろしくお願いします。
@Shick1112 すみません、すでにお気づきと思いますが、名前のタグはテストの動作確認のために、空のときはデフォルトでないようにいじってしまいました。 dcf7c4b
上記方針に合わせていただければと思います。よろしくお願いします。
@HidetoshiKawaguchi お返事ありがとうございます。 とりあえずは「省略しない」で実装を進めていき、何かあればまた相談したいと思います。
@yktsr Frame2Textのコードをなんとなく見て考えてただけなので気づきませんでした! 連絡ありがとうございます。
※ちょっと「文章の表示」の仕様を見て思ったことなんですが、 省略できるタグが四つあり、(Face、WindowPosition、Background、Name) 多く使うことがあるイベントなので省略した方が良いのかなーとも思ったんですが、 上記四つのタグは横並びに記載するのでtxtの行数がものすごく多くなるという心配もなさそうですね。
@yktsr
https://github.com/yktsr/Text2Frame-MV/pull/102 はもう完で、一回dev-220へmergeして良いものですか?
まだです。今コード内のヘルプドキュメントを書いているところで、それが終わったら完です。 終わりしだいこちらから連絡するので、お待ちください。 その際、改めてコードレビューをお願いします。
@HidetoshiKawaguchi @yktsr お疲れ様です。
Frame2Textの「文章の表示」のNameタグについてなのですが、以下の仕様で良いでしょうか? ■MVの場合 Nameタグを出力しない ⇒5つ目のパラメータが無い(undefined)で判断
■MZの場合 ・nameの値が有ると出力 ・nameの値が無い('')と出力しない
■Text2Frameの仕様
@Shick1112 Frame2TextがMVで実行されたか、MZで実行されたかによらず一律で、 parametersの5個目の要素が 長さ0の文字列(すなわち"") か、ない(すなわちparametersの要素数が4個)場合にNameタグを出力しない仕様が良いと思います。 具体例でいうと、提示頂いた2つの場合にはNameタグを出力せずに、それ以外の場合は出力するということになります。
{"code":101,"indent":1,"parameters":["",0,0,2]}
{"code":101,"indent":0,"parameters":["",0,0,2,""]}
それ以外の場合とはすなわち、
{"code":101,"indent":1,"parameters":["",0,0,2, "ハロルド"]}
や{"code":101,"indent":1,"parameters":["",0,0,2, "リード"]}
のようにparametersの5個目の要素に長さが1以上の文字列が入っていることを指します。
parametersの5個目の要素の状態は以下の3つでMECEです。そのため、以上の条件を使えばMV/MZのどちらで実行されているか判定せずに処理できるはずです。
Text2Frame同様、Frame2TextもMV/MZのどちらで実行しても結果は変わらないようにしたいです。 よろしくお願いします。
@Shick1112 すみません、1個上の私のコメントについてですが、ちょっと考えさせてください。ちょっと厄介かもしれません。
@HidetoshiKawaguchi 了解です。一旦他のところの実装を進めます。
※すみません あくまでパラメータ5個目で判断するっていうことを書きたかったのですが、 MVとMZの差等を色々書いてたらわかりにくい説明になってしまいました!
@Shick1112 改めて質問に回答します。まず、Nameタグの仕様についてですが、2つ前のコメントの通りでお願いします。いろいろと気になることがあって調べたり考えたりしたのですが、問題ないだろうという結論になりました。
その上で、1点お伝えします。
⇒よって、nameの値が無い場合はタグを出力しないようにするしかない?
こちらについてですが、nameの値が、parametersの5個目の要素を指しているのだとしたら、タグを出力しないようにするしかないですし、出力しないことが期待通りの動作です。私としてはそれ以外に考えられないので、ひょっとしたらShickさんのご懸念を拾いきれていない可能性があります。もし回答になっていなければ、もう少し詳細にご懸念を教えていただければと思います。
@HidetoshiKawaguchi 検討ありがとうございます。
回答してくれた通り、nameの値が無い場合は nameタグを出力しないようにするしかないということで懸念は解消しました! ⇒nameタグを出力しつつ、nameの値が無い記載があるかどうかの確認だったので。
@HidetoshiKawaguchi @yktsr お疲れ様です。 Text2Frameの「選択肢の表示」にて、既存バグと思われる個所を発見しました。 条件としては「デフォルト」の引数が「なし」且つ、「キャンセル」の値が「数字」の場合で起きているようです。 ※既存の部分なので一先ず連絡だけ致します。
タグの例:
<選択肢の表示: ウインドウ, 左, なし, 2>
実行値(parameters[2]が1)
{
"code": 102
"indent": 0
"parameters": [
[
"Yes"
"No"
]
1
1
2
2
]
}
期待値(parameters[2]が-1)
{
"code": 102
"indent": 0
"parameters": [
[
"Yes"
"No"
]
1
-1
2
2
]
}
「デフォルト」の引数が「なし」の時にも「exist_default_choice = true」を入れればいいと思うので、 7030行目の次の行に入れればいいのかな~と思っています。 ※勘違いしてたらすみません! https://github.com/yktsr/Text2Frame-MV/blob/824af13dcd387d26712d1ac027825cc58ff7cdf1/Text2Frame.js#L7019-L7041
以上です。確認よろしくお願いいたします。
@Shick1112 ありがとうございます。Issueとして発行しておきました。ver2.2.0のリリースまでに直しておきましょう。私がテストケースを作成しておくので、よろしければ実装の修正をお願いできないでしょうか?タイミングとしては、feature/add_all_tagがdev220ブランチに合流したあとが良いと思います。
@HidetoshiKawaguchi 了解です。 テストケースの作成よろしくお願いいたします。
@Shick1112 ありがとうございます。ただ、すみません。気楽に頼んでしまったのですが、よく考えたら既存機能のバグなことと、ShickさんはいまFrame2Textの実装をされており負担が大きいことを見逃しておりました。 一旦私か @yktsr が対応しますので、ShickさんはFrame2Textの実装に集中していただければと思います。もしまた手が足らなくなった場合は、再度お願いします。
@yktsr こちら、dev220がmasterに合流したときに間違えてクローズされたと思われます。リオープンするためにマージ先をmasterに変更しようとしても、私は権限の関係なのかできませんでした。そちらで対応できませんか?
すみません、dev-220 をmasterに合流させた後に、何も考えずにbranchを削除したらCloseされてしまいました。 Reopenして、base branch を master に変更しました。
@Shick1112 お疲れ様です。その後進捗はいかがでしょうか(といいつつローカルでテストを走らせて様子を伺ったりしております)
Frame2Text実現の過程としてText2Frameの全イベントコマンドのタグ実装にご協力いただきありがとうございました。改めてお礼申し上げます。これまでアップデート告知は何度もしているのですが、今までで一番大きな反響がありました。やはり部分的にテキストで書けるのと、100%すべて書けるようになるのではインパクトが大きく違うようです。私と yktsr の2人では、実現のための工数があまりにも大きく取り掛かろうとも思いませんでしたが、 Shickさんに仕様と実装の草案をお持ち込みいただいたおかげで、重い腰を上げることができました。また、その後も私の小姑のような細かな指摘・修正にも対応いただけましたね。本当にありがとうございます。何度お礼をいっても飽き足りません。
さて、Frame2Textの開発についてですが、ご不明点がある場合や、なかなか進捗が出せない時は遠慮なくその旨お伝えいただければと思います。私から急かすことはしませんので、ご自身のタイミングで何かしらご連絡ください。そのご連絡の内容やタイミングをみて、ドキュメント作成について議論させていただければと思います。一応ドキュメントについては私が責任を持っているので、Frame2Textについても、私の方で書こうかと考えております(ヘルプドキュメントにはプラグインコマンド以外ほとんど記載がありませんよね?)。また、Text2Frame側のヘルプドキュメントや、Wikiにも記載する予定です。詳しくは、また議論させてください。
ではでは実装のほう、よろしくお願いいたします。
@HidetoshiKawaguchi @yktsr お疲れ様です。お返事遅れました。
お二人にはこちらが雑にプルリクエストしたものを確認してもらい、作業しやすいように整えてくれました。 私だけではここまでしっかりとした実装・仕様決めは出来ませんでした。 本当にありがとうございます。
Frame2Textについて 現在実装していますが、一つ確認したいことがあります。 feature/Frame2TextにはText2Frameの既存バグの修正やテストケース等が入れ込まれていない状態だと思うのですが、 どう入れ込めばいいでしょうか? ※Gitの仕様に詳しくなく、変に入れ込むと面倒になる可能性があったので・・・
Frame2Textのドキュメントについて そうですね、現状プラグインパラメータの記載しかない状態です。 自分の考えとしては基本的な使い方はText2Frameと変わらず、指定したイベントのテキストを出力するだけなので 使い方と、Frame2Textのみの仕様を記載すればいいと思っています。 Frame2Textのみの仕様はEnglishTag(出力するテキストの英語/日本語)だけだったはずなので、 その辺りの記載をすればいいのかなーと思っています。
一週間ほどFrame2Textの作業が止まっていましたが、土日でまた作業が出来ると思います。
以上です。よろしくお願いいたします。
@Shick1112 お返事ありがとうございます。
Text2Frameの取り込みについてですが、@yktsr からどうすればいいか伝えてもらえると思います。(シレッと、このブランチにすでにmasterがマージされているような気がするのでもう問題ないような気がしますが・・・。pull すると反映されると思います)
使い方と、Frame2Textのみの仕様を記載すればいいと思っています。
はい、そうですね。それでいいと思います。ご自身で書きたいものについては書いていただき、その後私のほうで追記したいことを追記するという形をとらせてください。 よろしくおねがいします。
このbranchへ、masterをmergeしておきました。
git pull origin
で、最新が取り込めると思いますので、お試しください。 手元で変更されているのはFrame2Textだけだと思うので、特に衝突はなく取り込めると思います。
もし、まだコミットしていない修正と衝突する場合は、
git stash save
で一旦変更を退避し、
git pull origin
でmergeして、
git stash pop
で、最新を取り込んだ上で、コミットしていない修正と合体できると思います。
@HidetoshiKawaguchi 了解です! 実装が終わったらドキュメントの記載もしてみます~
@yktsr mergeありがとうございます。問題なくpullできました!
以上です。よろしくお願いいたします。
@HidetoshiKawaguchi 実装とドキュメントの記載が完了しました。 確認・追記をお願いします!
※Text2Frameの使い方の書き方に合わせるとこんな感じかな~という感じで記載しました。 時期が時期なので、お手すきの際に見ていただければ~
@Shick1112 ありがとうございます!そして長大な作業お疲れ様です! はい、年末年始にかけていずれかでレビューさせていただきます。 ドキュメントに関しては、気になる点はこちらで直すと思います。予めご承知いただければ幸いです。 ではではひとまずお待ちください。よろしくお願いします
@Shick1112 おつかれさまです!テストを見たところ、Text2Frameの方のテストの、Flow Congrol / Conditional Branch/Buttonでエラーが起きていますね(npm run testで確認可能)。test_frame2textは確かに問題ないです。
に追記があるようなのですが、ひとまずこちらの修正をお願いできないでしょうか。
ドキュメント等の確認はまだ実施できておりません。明日から私用で作業時間がしばらく取れなく、下手したら1月中旬くらいまで音沙汰がなくなるかもしれません。よろしくお願いします。
何がしかの既知の不具合っぽいのを指摘いただいたようなので自分修正入れましょうか。
@HidetoshiKawaguchi @yktsr 修正しました。 私が追加したテストケースの想定結果に不備があり、そのせいでtext2Frameのテストで問題が出ていました。 ⇒button.txtには「[キャンセル]がトリガーされている場合」と「% [シフト]がリピートされている場合」を二つずつ追加していたが、 expected_button.jsonには一つずつしか追加していなかった。 ※大変お騒がせしました!
対応ありがとうございます!
@Shick1112 修正ありがとうございます! ほんの少しだけ作業時間をとれたので、超簡単な動作確認をしてみました。今の所問題なさそうです。 来週の1/8~1/12に少し作業時間がとれそうなので、その際にFrame2Text.js内のヘルプドキュメントの修正やwiki用のドキュメント、README.mdの記述をしようと思います。また、実機での(MV・MZ両方)網羅的な機能の動作確認も実施します。
1/12までに何かしらの報告をするつもりなので、それまでお待ちください。 (ある程度リリースの目処がたったところで、今後のメンテナンス体制についての話もさせてください。Frame2Textリリース後の、Shickさんの関わり方についての希望をお聞きしたいです) よろしくお願いします。
@Shick1112 https://github.com/Shick1112/Text2Frame-MV/pull/37 動作確認およびドキュメントの整形を行いました。詳しくは上述したプルリクをご覧ください。 よろしければそちらでマージをしていただければ、私としてはいつでもオリジナルのmasterブランチに合流=リリースしても良い状態だという認識です。
それとは別に、リリース後のメンテナンス体制についてお話させてください。 Text2FrameとFrame2Textは相互変換をするプラグイン同士です。どちらかを修正すると、どちらかも修正の必要があるというケースも結構あるのではないかと踏んでいます。なので、例えばText2Frameは@yktsr か私、Frame2TextはShickさんというように明確に分担はしないほうが良いと考えております。 そういうこともあり、現時点では明確な役割分担はせずに、何か問題があったり追加機能の要望があった時に、都度誰が担当するかは考えようと思います。ただ基本方針としてはFrame2Textに関する問題の修正や追加機能、アップデートはShickさんにお願いしようと考えております。Github上でメンションしたりしてお呼びしようと思いますが、期間があくと見逃す場合も多くなってくることでしょう。その場合はX(旧Twitter)のいなずまそふとさんのアカウントのDM経由で連絡をいれさせてください。 Shickさんが対応出来ない場合は@yktsr か私がメンテもしますが、もしもShickさんと連絡がとれなくなり、またFrame2Textの修正が難しい状況に陥った場合はこちらの判断でサポート・メンテナンスを停止する可能性があることは、予めご承知いただければと思います(とはいえそうならないよう善処するつもりではあります)。
私としては、リリース後の方針は以上にしようと思うのですが、何かご意見や要望はありますでしょうか?よろしくお願いします。
@HidetoshiKawaguchi ドキュメントの整形ありがとうございます! 後ほど確認致します。
メンテナンスの基本方針としては記載していただいた通りでいいと思います。 その上で、私がすぐに対応できない場合は相談させてください。
また、連絡方法もそれでお願いいたします。
以上です。よろしくお願いいたします。
@Shick1112 メンテナンスの基本方針について合意いただきありがとうございます。 また、https://github.com/Shick1112/Text2Frame-MV/pull/37 にてドキュメントの整形も受け入れていただきありがとうございます。
@yktsr Frame2Textに関する全作業が終わりました。レビューしていただき、問題なければmasterに合流したいので、よろしくお願いします。wikiのドキュメント作成も完了しています。
作業ありがとうございました! Approveするまでに、何点か対応しておきたいことがあります。基本、自分が作業やります。
改めて、作業ありがとうございます! Merge までもうしばらくお付き合いください。
wikiをさっと確認した感じだと、現状1イベント単位名での処理になるので、全イベントの一括出力 「EXPORT_ALL_EVENT_TO_MESSAGES」はあってもいいかな、と思いますが、どうでしょう。 そこそこの検討がいりますが、1.0で以降でも。
@yktsr お返事遅れました!
基本的に、レポジトリがMIT LicenseなのでMITとして公開することになります。すでに表記がありますが、念の為確認です。 > @Shick1112
はい、MITで問題ありません。
著作権者の名称が必要です。Shick と表記するのが適当かと思いますが、ご希望を教えてください。
記載している通り、Shickでお願いします。
以上です。よろしくお願いします。
wikiをさっと確認した感じだと、現状1イベント単位名での処理になるので、全イベントの一括出力 「EXPORT_ALL_EVENT_TO_MESSAGES」はあってもいいかな、と思いますが、どうでしょう。 そこそこの検討がいりますが、1.0で以降でも。
いろいろと出来ることはありますよね。出力ファイル名をどうするかとか、仕様策定がまぁまぁ大変だと思いますので、やるのであればリリース後でいいと思います
Text2FrameをプルリクしたShickです。 Text2Frameの逆の処理(イベント⇒テキスト)をするFrame2Textを作成しました。
こちらのレビューもしていただけたら嬉しいです!
以上です。よろしくお願いします。