パラグラフにCodeブロックを含む時の問題を修正 #90

ksato9700 · 2021-05-09T03:08:47Z

#89 の問題を解決してみました。できるだけ既存のコードに手を入れないようにしたので少し冗長な部分もありますが、Codeブロックの中の不一致も含めてチェック出来るようにしたつもりです。

azu · 2021-05-09T03:42:59Z

src/util/pair-checker.js

@@ -55,21 +59,35 @@ export function checkPair(context, { left, right }) {
            currentStrInParagraph = [];
            isInParagraph = true;
        },
+        [Syntax.Code](node) {


この Code nodeでの探索ってコード内の括弧の対応を見ています?

「text

Codeはinvalidなケースが正の場合があるので意図的に無視していました。
間違い例とかを出すケースなどがあるので、オプショナルにしないと行けない感じがします。

そうですね。Code内の対応付をとりあえず見ないということであればその部分を削除してしまって良いかと思います。問題なければそれでPR出し直します。

はい、コード内の括弧チェックは別対応でした方が良いと思います。

test/4.3.5-test.js

azu · 2021-05-09T06:40:31Z

src/util/pair-checker.js

+            if (!isInParagraph) {
+                return;
+            }
+            currentStrInParagraph.push(node);


**Strong** "text

InlineのNodeにはCode以外にもStrongなど色々あるので、他のNodeが挟まると同じ問題が起きそうな気がします。
(全部のNodeを書くとNodeが増えたときに問題があるので、pushするのはStr nodeのみで解決するのが良い気はします)

reportするNodeをStrにして、foundMissingPairNodesで返すindexはそのStr nodeからのrelativeなindexにする(ペアの探索の仕方をちょっと工夫する)
OR
reportするNodeはParagraphにして、Str + Str のようにStr node同士を結合を作るときに、

Str1 Str1

Str1のrangeが[0,4]

Str2のrangeが[10,14]

4 - 10 をスペースで埋める

みたいに Strのrangeを見て空いている場所は、スペースなどで自動的にpaddingで埋めるとかですかね(今のPRの実装はこれに近い)?

そうですね。その方向で再実装してみます。コメントありがとうございます。

ksato9700 · 2021-05-09T15:07:35Z

やや複雑になってしまったかもですが、再実装してみました。一度全てのleft rightの文字のある場所をリスト化し、leftとrightが同じであるか否かによって処理を分けています。カラムは正しいnodeを report()に渡せば計算してくれるようです。

azu · 2021-05-09T23:57:25Z

src/util/pair-checker.js

+                    while (index < text.length) {
+                        index = text.indexOf(right, index);
+                        if (index < 0) break;
+                        symbolLocations.push({
+                            index,
+                            node,
+                            type: "right"
+                        });
+                        index += 1;
+                    }


この text から特定のsymbolを探索する処理を left と right でまとめられそうな気がします。
findAllSymbolLocations({ symbol, text }): symbolLocation[] みたいな感じで、特定のsymbolを探して、そのlocationの配列を返す感じですね。
(名前はもっといいのがあるかもしれないですが。tc39/proposal-regexp-match-indices: ECMAScript RegExp Match Indicesとかは近い話かも)

擬似コード的には次のようにまとめられる気がします。

const leftSymbolLocations = findAllSymbolLocations({ symbol: left, text }); const rightSymbolLocations = left !== right ? findAllSymbolLocations({ symbol: right, text }) : []; const allSymbolLocations = [...leftLocations, ...rightLocations].sort((a, b) => a.index - b.index);

📝 type=bothみたいのを入れるべきかはまよったけど、bothを入れてもそこまでシンプルにはならないのかな。
(後半のチェックの処理でleft !== rightで処理を分けずにやるにはbothがあるといいけど、あんまりシンプルにできないかもと思った)

azu · 2021-05-10T00:12:38Z

src/util/pair-checker.js

+                    unmatchParences.push(item);
+                } else {
+                    // right
+                    let last = unmatchParences.pop();


lastUnmatchParences とかmatchかunmatchが迷うので名前つけたほうが良さそうですね。

azu · 2021-05-10T00:13:03Z

src/util/pair-checker.js

+        } else {
+            let unmatchParences = [];
+            while (matchParentheses) {
+                let item = matchParentheses.shift();


constで大丈夫なはずですね。

Suggested change

let item = matchParentheses.shift();

const item = matchParentheses.shift();

azu · 2021-05-10T00:13:18Z

src/util/pair-checker.js

-                matchParentheses.pop();
-                foundLeft = false;
+        } else {
+            let unmatchParences = [];


Suggested change

let unmatchParences = [];

const unmatchParences = [];

popやshiftなのでconstでいいはず

azu · 2021-05-10T00:16:12Z

src/util/pair-checker.js

+            .flat();
+
+        if (left === right) {
+            if (matchParentheses.length % 2 == 0) {


matchParentheses.length % 2 == 0 を変数にしたいですね。

const isCompletedParentheses = matchParentheses.length % 2 == 0;

みたいな(名前はてきとうですが)

azu · 2021-05-10T14:17:41Z

src/util/pair-checker.js

-                foundLeft = false;
+        } else {
+            const lastUnmatchParences = [];
+            while (matchParentheses) {


matchParentheses って falseになることってありますか? ( break でなんとか抜けているのか)
while(true) になっている気がします。
matchParentheses.length !== 0 が意図した形かもしれないです。

仰るとおりです。ご指摘ありがとうございます。

azu

LGTM

対応ありがとうございます！

パラグラフにCodeブロックを含む時の問題を修正

82b17c7

azu reviewed May 9, 2021

View reviewed changes

test/4.3.5-test.js Show resolved Hide resolved

カラムがズレる問題を修正しました。

c863ac3

azu reviewed May 9, 2021

View reviewed changes

Str nodeのみをpushする方針で再実装してみました。

3866604

azu reviewed May 10, 2021

View reviewed changes

助言に従いリファクタリング

d97168c

azu reviewed May 10, 2021

View reviewed changes

loopの判定条件のバグを修正

0d4fdda

azu approved these changes May 11, 2021

View reviewed changes

azu merged commit 3c90be6 into textlint-ja:master May 11, 2021

ksato9700 deleted the doublequote-with-code branch May 12, 2021 01:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

パラグラフにCodeブロックを含む時の問題を修正 #90

パラグラフにCodeブロックを含む時の問題を修正 #90

ksato9700 commented May 9, 2021

azu May 9, 2021

ksato9700 May 9, 2021

azu May 9, 2021 •

edited

Loading

azu May 9, 2021 •

edited

Loading

ksato9700 May 9, 2021

ksato9700 commented May 9, 2021

azu May 9, 2021 •

edited

Loading

azu May 10, 2021

azu May 10, 2021

azu May 10, 2021

azu May 10, 2021

azu May 10, 2021 •

edited

Loading

ksato9700 May 10, 2021

azu left a comment

	let item = matchParentheses.shift();
	const item = matchParentheses.shift();

パラグラフにCodeブロックを含む時の問題を修正 #90

パラグラフにCodeブロックを含む時の問題を修正 #90

Conversation

ksato9700 commented May 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azu May 9, 2021 • edited Loading

Choose a reason for hiding this comment

azu May 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksato9700 commented May 9, 2021

azu May 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azu May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azu left a comment

Choose a reason for hiding this comment

azu May 9, 2021 •

edited

Loading

azu May 9, 2021 •

edited

Loading

azu May 9, 2021 •

edited

Loading

azu May 10, 2021 •

edited

Loading