feat: Change automatic line breaks to optional #64

akabekobeko · 2021-01-17T09:40:11Z

#50 対応。

akabekobeko · 2021-01-17T09:46:35Z

今回の PR から API やプロパティーなどにコードドキュメントを付けるようにしました。コードが自己言及的に書かれていればコメントは不要という思想もありますが、最もコードに近い場所へ意図などを記述する場としてコメントはあったほうがよいと考えているため、今後はなるべく書く予定です。

IDE や VS Code など入力補完の効く環境下で API リファレンスとして機能することも狙っています。

akabekobeko · 2021-01-18T09:04:22Z

すみません。CLI 対応が抜けてたので改めてコミットしました。もし CLI を試される場合は sample.md を以下の内容で作成して

テスト
テスト

VFM を npm link した後に sample.md の置かれた場所で

$ vfm sample.md

と

$ vfm sample.md --auto-line-breaks

を比較してください。既定値 false なのでオプション未指定はそのまま、指定すると改行位置に <br>　が挿入されます。

akabekobeko · 2021-01-18T09:07:28Z

↑のコメントを書いて思ったのですが remark-breaks の処理は「変換」ではなく、改行位置へ <br> を挿入して元の改行は残るため「変換」以外で表現したほうがよいのかもしれません。

コードドキュメントや CLI ヘルプでは "Convert" としましたが、なにかよい代替表現があればご指摘ください。またオプションもよりよい名前がありそうですね。

spring-raining

PRありがとうございました。JSDocによるコメントなどありがたいです！

レビューコメント以外に一点、autoLineBreaks の有無による出力結果の変化についてテストがない状態なので、tests/block/ 以下に追加していただけるとありがたいです。
例えばGFMでは、改行1つによるautoLineBreaksはoffですが、行末にや \ があると改行される仕様があります (https://gist.github.com/shaunlebron/746476e6e7a4d698b373 )。その点なども、testで確認できると良いかと思いました。

spring-raining · 2021-01-18T17:02:25Z

src/revive-parse.ts

+
+  if (options.autoLineBreaks) {
+    // Line breaks are processed immediately after ruby
+    results.splice(3, 0, breaks);


indexをマジックナンバーで指定して挿入する実装は results の変更への追従を忘れてしまう恐れがあるためおすすめできません。同等の実装は以下のコードでも可能です。

const results = [ (snip) ...(options.autoLineBreaks ? [breaks] : []), (snip) ];

spread operator で条件分岐いけるのですね。マジックナンバーよりも好ましいのでこの方法に変更します。

spring-raining · 2021-01-18T17:11:00Z

src/revive-parse.ts

+ * @param options Options.
+ * @returns Parsers.
+ */
+export const reviveParse = (options: MarkdownOptions) => {


返値の型として unified.PluggableList<unified.Settings> を明示していただけますか？その結果、results にある as unified.PluggableList<unified.Settings> を省略することが可能です。

元コードで直に配列を返す際の as を踏襲していましたが、関数化したなら確かに返り値の型で明示するほうが好ましいですね。そのように変更します。

akabekobeko · 2021-01-18T22:20:14Z

autoLineBreaks のテストですが、追加しようとしたら index.test.ts に handle hard line break という名で存在していたためやめました。ただしこれは「常時 remark-breaks が実行されること」を想定して index.test.ts に定義されているようですので、

tests/block/ 以下に追加していただけるとありがたいです。

で対応します。

… option If it is not expanded and is a normal argument, the caller must resolve the dependency between the properties. The VFM function avoids this by expansion, so referred to it.

1. Avoid `as` by explicitly specifying the return type of reviveParse 2. Independent test of line break handle

akabekobeko · 2021-01-18T23:56:15Z

指摘のあった箇所を修正しました。reviveParse は返り値における型の明示ついでに処理が return のみとなったため、略記にしてあります。

akabekobeko · 2021-01-18T23:57:16Z

現時点の対応でよろしければ merge しますので、お手すきの時に再レビューをお願いします。

MurakamiShinyu · 2021-01-19T16:32:47Z

--auto-line-breaks という名前ですが、長い行を自動で改行を入れるという意味かと誤解されるかもしれません。

Pandocはオプションで改行の扱いを変えることができるので、それに倣うのがよいと思います。

https://pandoc.org/MANUAL.html#extension-hard_line_breaks

Extension: hard_line_breaks

Causes all newlines within a paragraph to be interpreted as hard line breaks instead of spaces.

Extension: ignore_line_breaks

Causes newlines within a paragraph to be ignored, rather than being treated as spaces or as hard line breaks. This option is intended for use with East Asian languages where spaces are not used between words, but text is divided into lines for readability.

Extension: east_asian_line_breaks

Causes newlines within a paragraph to be ignored, rather than being treated as spaces or as hard line breaks, when they occur between two East Asian wide characters. This is a better choice than ignore_line_breaks for texts that include a mix of East Asian wide characters and other characters.

テスト:

$ pandoc --from=markdown+hard_line_breaks

The quick brown fox
jumps over the lazy dog.

↓

<p>The quick brown fox<br />
jumps over the lazy dog.</p>

ということで、--auto-line-breaks を --hard-line-breaks に（autoLineBreaks を hardLineBreaks に）変更することを提案します。

akabekobeko · 2021-01-19T22:27:52Z

ということで、--auto-line-breaks を --hard-line-breaks に（autoLineBreaks を hardLineBreaks に）変更することを提案します。

いい案ですね。対応します。

akabekobeko · 2021-01-19T22:52:41Z

村上さんのオプション名案を反映しました

akabekobeko · 2021-01-20T11:02:28Z

@spring-raining @MurakamiShinyu
現時点のコードでよろしければ、その旨をお知らせください。merge したいと思います。

spring-raining · 2021-01-20T13:42:08Z

README.md

@@ -24,6 +24,7 @@ Vivliostyle Flavored Markdown (VFM), a Markdown syntax optimized for book author
    - [`partial` (default: `false`)](#partial-default-false)
    - [`title` (default: `undefined`)](#title-default-undefined)
    - [`language` (default: `undefined`)](#language-default-undefined)
+    - [`autoLineBreaks` (default: `false`)](#autoLineBreaks-default-false)


Suggested change

- [`autoLineBreaks` (default: `false`)](#autoLineBreaks-default-false)

- [`hardLineBreaks` (default: `false`)](#hardLineBreaks-default-false)

spring-raining · 2021-01-20T13:42:34Z

README.md

@@ -210,6 +211,38 @@ will generates:
 </html>
 ```

+#### `autoLineBreaks` (default: `false`)


Suggested change

#### `autoLineBreaks` (default: `false`)

#### `hardLineBreaks` (default: `false`)

spring-raining · 2021-01-20T13:42:45Z

README.md

+new
+line
+`,
+  { autoLineBreaks: true },


Suggested change

{ autoLineBreaks: true },

{ hardLineBreaks: true },

akabekobeko · 2021-01-20T14:17:10Z

@spring-raining ありがとうございます。指摘のあった README の変更漏れを修正しました。

spring-raining

良さそうです！

akabekobeko · 2021-01-20T14:27:38Z

@spring-raining @MurakamiShinyu レビューありがとうございました！

MurakamiShinyu

LGTM

akabekobeko added 2 commits January 17, 2021 18:37

feat: Change automatic line breaks to optional

bebc736

docs: Change automatic line breaks to optional

51afc52

akabekobeko added docs enhancement labels Jan 17, 2021

akabekobeko self-assigned this Jan 17, 2021

akabekobeko requested a review from spring-raining January 17, 2021 09:40

feat: Added --auto-line-breaks option to CLI

24fe380

spring-raining requested changes Jan 18, 2021

View reviewed changes

akabekobeko added 2 commits January 19, 2021 08:33

feat: Changed to keep the default value per property by expanding the…

3a691f6

… option If it is not expanded and is a normal argument, the caller must resolve the dependency between the properties. The VFM function avoids this by expansion, so referred to it.

fix: Fixed by PR review

417e944

1. Avoid `as` by explicitly specifying the return type of reviveParse 2. Independent test of line break handle

akabekobeko requested a review from spring-raining January 19, 2021 05:08

MurakamiShinyu mentioned this pull request Jan 19, 2021

Change hard line breaks to optional #50

Closed

feat: Renamed line breaks option

494185a

spring-raining reviewed Jan 20, 2021

View reviewed changes

fix: Apply option name changes to README

e6c4431

spring-raining approved these changes Jan 20, 2021

View reviewed changes

akabekobeko merged commit 922eb4b into master Jan 20, 2021

akabekobeko deleted the #50 branch January 20, 2021 14:27

MurakamiShinyu approved these changes Jan 20, 2021

View reviewed changes

akabekobeko added this to the v1.0.0 milestone Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Change automatic line breaks to optional #64

feat: Change automatic line breaks to optional #64

akabekobeko commented Jan 17, 2021

akabekobeko commented Jan 17, 2021

akabekobeko commented Jan 18, 2021

akabekobeko commented Jan 18, 2021

spring-raining left a comment

spring-raining Jan 18, 2021

akabekobeko Jan 18, 2021

spring-raining Jan 18, 2021

akabekobeko Jan 18, 2021

akabekobeko commented Jan 18, 2021

akabekobeko commented Jan 18, 2021

akabekobeko commented Jan 18, 2021

MurakamiShinyu commented Jan 19, 2021 •

edited

Loading

Extension: `hard_line_breaks`

Extension: `ignore_line_breaks`

Extension: `east_asian_line_breaks`

akabekobeko commented Jan 19, 2021

akabekobeko commented Jan 19, 2021

akabekobeko commented Jan 20, 2021

spring-raining Jan 20, 2021

spring-raining Jan 20, 2021

spring-raining Jan 20, 2021

akabekobeko commented Jan 20, 2021

spring-raining left a comment

akabekobeko commented Jan 20, 2021

MurakamiShinyu left a comment

	- [`autoLineBreaks` (default: `false`)](#autoLineBreaks-default-false)
	- [`hardLineBreaks` (default: `false`)](#hardLineBreaks-default-false)

	#### `autoLineBreaks` (default: `false`)
	#### `hardLineBreaks` (default: `false`)

feat: Change automatic line breaks to optional #64

feat: Change automatic line breaks to optional #64

Conversation

akabekobeko commented Jan 17, 2021

akabekobeko commented Jan 17, 2021

akabekobeko commented Jan 18, 2021

akabekobeko commented Jan 18, 2021

spring-raining left a comment

Choose a reason for hiding this comment

spring-raining Jan 18, 2021

Choose a reason for hiding this comment

akabekobeko Jan 18, 2021

Choose a reason for hiding this comment

spring-raining Jan 18, 2021

Choose a reason for hiding this comment

akabekobeko Jan 18, 2021

Choose a reason for hiding this comment

akabekobeko commented Jan 18, 2021

akabekobeko commented Jan 18, 2021

akabekobeko commented Jan 18, 2021

MurakamiShinyu commented Jan 19, 2021 • edited Loading

Extension: hard_line_breaks

Extension: ignore_line_breaks

Extension: east_asian_line_breaks

akabekobeko commented Jan 19, 2021

akabekobeko commented Jan 19, 2021

akabekobeko commented Jan 20, 2021

spring-raining Jan 20, 2021

Choose a reason for hiding this comment

spring-raining Jan 20, 2021

Choose a reason for hiding this comment

spring-raining Jan 20, 2021

Choose a reason for hiding this comment

akabekobeko commented Jan 20, 2021

spring-raining left a comment

Choose a reason for hiding this comment

akabekobeko commented Jan 20, 2021

MurakamiShinyu left a comment

Choose a reason for hiding this comment

MurakamiShinyu commented Jan 19, 2021 •

edited

Loading

Extension: `hard_line_breaks`

Extension: `ignore_line_breaks`

Extension: `east_asian_line_breaks`