feat: Enabled syntax highlight to a code block #83

korosuke613 · 2020-07-01T02:53:18Z

closed #82

korosuke613 · 2020-07-01T04:06:03Z

CI通るように修正します

korosuke613 · 2020-07-09T08:29:19Z

conflict直してpushしなおします。

ganta

@korosuke613
見ました！ご確認よろしくです〜

src/app/markdown/renderer/highlight-settings.ts

src/app/markdown/renderer/marktone-renderer.ts

korosuke613 · 2020-07-12T06:47:43Z

@ganta
丁寧なコードレビューありがとうございます！とても勉強になります。
レビュー内容を反映しました。
よろしくお願いします。

ganta

@korosuke613 見逃しがあったので、いくつかコメントしました〜

src/app/markdown/renderer/marktone-renderer.ts

codeclimate · 2020-07-14T01:51:04Z

src/app/markdown/renderer/marktone-renderer.ts

@@ -20,6 +22,43 @@ class MarktoneRendererHelper {

    return html;
  }
+
+  static unescapeHTML(html: string): string {


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2020-07-14T02:38:41Z

src/app/markdown/renderer/marktone-renderer.ts

-    const escapeTest = /[&<>"']/;
-    const escapeReplace = /[&<>"']/g;
-    const replacements: { [key: string]: string } = {
+  private static escapeSettings: ReplaceSettings = {


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2020-07-14T02:38:41Z

src/app/markdown/renderer/marktone-renderer.ts

+    },
+  };
+
+  private static unescapeSettings: ReplaceSettings = {


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2020-07-14T02:38:41Z

src/app/markdown/renderer/marktone-renderer.ts

+    return target;
+  }
+
+  static escapeHTML(html: string): string {


Similar blocks of code found in 2 locations. Consider refactoring.

src/app/markdown/renderer/marktone-renderer.ts

korosuke613 · 2020-07-14T02:45:28Z

@ganta
五十嵐さん。DRYの精神でescapeHTML()とunescapeHTML()の共通コードを抜き出して、なるべく短くしたのですが、それでもcodeclimateにsimilar blocksがあると怒られてしまいます...
何かいい方法はないでしょうか？

https://codeclimate.com/github/ganta/chrome-extension-marktone/pull/83

ganta · 2020-07-14T03:08:15Z

@korosuke613
DRYの罠にハマりましたね〜

コード類似の指摘は、大抵のケースにおいて似て非なるものであることが多いです。
そのため、静的解析の指摘を素直にそのまま対応しようとすると、逆にコードが複雑化したりといった罠に陥ります。
こういう場合は重複していることを自体を解決するのではなく、どこかが根本的におかしいことを疑ったほうがよいです。

今回のケースではhighlight.js側がエスケープ処理の有無に対応していないことを回避しようとしたことが発端ですね。
もっというと、Marked.jsがエスケープの状態を引数で取り回してくる仕様も、結構違和感があります。
(引数にOn/Offを渡すのは複雑化するので避けるべきパターンです)

と言っても、現段階ではわりとどうしようもない感じがするので、
最初のシンプルなescapeHTML()とunescapeHTML()が独立して存在している状態にしましょうか。
おそらくそれがいちばん可読性が高そうです。

レビューが完了したらこちらでCode Climateの指摘を除外します。

コードのあやしいところを見つけてくれるので、コード類似指摘自体はまだ有効化したままにしておこうと思います。

korosuke613 · 2020-07-14T04:27:52Z

@ganta
なるほど〜！
根本的な原因を取り除くことを考えた方が良いと

根本原因はライブラリ側にあるので、確かに厳しい感じはありますね〜

最初のシンプルなescapeHTML()とunescapeHTML()が独立して存在している状態にしましょうか。
おそらくそれがいちばん可読性が高そうです。

了解です！修正しました！
よろしくお願いします。

ganta

@korosuke613 ちょっとだけコメントしましたのでよろしくです〜

ganta · 2020-07-16T04:25:29Z

src/app/markdown/renderer/marktone-renderer.ts

+    const escapeTest = /((&lt)|(&amp;)|(&gt;)|(&quat)|(&#39))/;
+    const escapeReplace = new RegExp(escapeTest, "g");


[nits]
ここらへんの変数名は意味が逆になっているのでunescapeTest、unescapeReplaceというようにしておきましょうか
(もとの命名がよろしくないというのはあるのですが)

あああ確認不足でした...
修正しました！ありがとうございます！
https://github.com/ganta/chrome-extension-marktone/pull/83/files#diff-9eaf230ebb45489bd5b70a3e292c7cc5R27-R28

ganta · 2020-07-16T04:29:56Z

src/app/markdown/renderer/marktone-renderer.ts

@@ -20,6 +22,43 @@ class MarktoneRendererHelper {

    return html;
  }
+
+  static unescapeHTML(html: string): string {
+    const escapeTest = /((&lt)|(&amp;)|(&gt;)|(&quat)|(&#39))/;


[MUST]
いくつかセミコロンが抜けてますね。

&と;だけ別にする書き方もできます

Suggested change

const escapeTest = /((&lt)|(&)|(>)|(&quat)|(&#39))/;

const escapeTest = /(&(?:lt|amp|gt|quat|#39);)/;

テスト不足でした...
確かに&と;は共通してましたね。ありがとうございます！
https://github.com/ganta/chrome-extension-marktone/pull/83/files#diff-9eaf230ebb45489bd5b70a3e292c7cc5R27

ganta · 2020-07-17T02:59:49Z

src/app/markdown/renderer/marktone-renderer.ts

@@ -20,6 +22,43 @@ class MarktoneRendererHelper {

    return html;
  }
+
+  static unescapeHTML(html: string): string {
+    const unescapeTest = /(&(?:lt|amp|gt|quat|#39);)/;


@korosuke613
quotのtypo見逃していました〜
他は大丈夫そうです！

oh...すみません確認不足でした..
修正しました！
https://github.com/ganta/chrome-extension-marktone/pull/83/files#diff-9eaf230ebb45489bd5b70a3e292c7cc5R27

ganta

@korosuke613 Thank you!

korosuke613 marked this pull request as draft July 1, 2020 04:05

korosuke613 force-pushed the enable-syntax-highlight branch from b136284 to c38ddec Compare July 1, 2020 04:23

korosuke613 marked this pull request as ready for review July 1, 2020 04:24

ganta self-requested a review July 1, 2020 04:30

korosuke613 force-pushed the enable-syntax-highlight branch from c38ddec to 7376d26 Compare July 9, 2020 08:19

korosuke613 force-pushed the enable-syntax-highlight branch from 7376d26 to 4cf9af3 Compare July 9, 2020 08:33

ganta requested changes Jul 11, 2020

View reviewed changes

korosuke613 force-pushed the enable-syntax-highlight branch from 4cf9af3 to 1930df7 Compare July 12, 2020 06:37

ganta requested changes Jul 13, 2020

View reviewed changes

src/app/markdown/renderer/marktone-renderer.ts Outdated Show resolved Hide resolved

src/app/markdown/renderer/marktone-renderer.ts Show resolved Hide resolved

korosuke613 force-pushed the enable-syntax-highlight branch from 1930df7 to c41f38a Compare July 14, 2020 01:50

codeclimate bot reviewed Jul 14, 2020

View reviewed changes

korosuke613 force-pushed the enable-syntax-highlight branch 4 times, most recently from bd78320 to 7a4a791 Compare July 14, 2020 02:38

codeclimate bot reviewed Jul 14, 2020

View reviewed changes

korosuke613 commented Jul 14, 2020

View reviewed changes

src/app/markdown/renderer/marktone-renderer.ts Show resolved Hide resolved

korosuke613 force-pushed the enable-syntax-highlight branch from 7a4a791 to 0467cb4 Compare July 14, 2020 02:48

korosuke613 force-pushed the enable-syntax-highlight branch 2 times, most recently from 64646f3 to a708324 Compare July 14, 2020 04:26

ganta requested changes Jul 16, 2020

View reviewed changes

korosuke613 force-pushed the enable-syntax-highlight branch from a708324 to 01fdb72 Compare July 16, 2020 12:21

ganta reviewed Jul 17, 2020

View reviewed changes

feat: Enabled syntax highlight to a code block

7c90b1e

korosuke613 force-pushed the enable-syntax-highlight branch from 01fdb72 to 7c90b1e Compare July 17, 2020 04:13

ganta approved these changes Jul 17, 2020

View reviewed changes

ganta merged commit 746eb63 into ganta:master Jul 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enabled syntax highlight to a code block #83

feat: Enabled syntax highlight to a code block #83

korosuke613 commented Jul 1, 2020 •

edited

Loading

korosuke613 commented Jul 1, 2020

korosuke613 commented Jul 9, 2020

ganta left a comment

korosuke613 commented Jul 12, 2020

ganta left a comment

codeclimate bot Jul 14, 2020

codeclimate bot Jul 14, 2020

codeclimate bot Jul 14, 2020

codeclimate bot Jul 14, 2020

korosuke613 commented Jul 14, 2020

ganta commented Jul 14, 2020

korosuke613 commented Jul 14, 2020

ganta left a comment

ganta Jul 16, 2020

korosuke613 Jul 16, 2020

ganta Jul 16, 2020

korosuke613 Jul 16, 2020

ganta Jul 17, 2020

korosuke613 Jul 17, 2020

ganta left a comment

		const escapeTest = /((&lt)\|(&)\|(>)\|(&quat)\|(&#39))/;
		const escapeReplace = new RegExp(escapeTest, "g");

	const escapeTest = /((&lt)\|(&)\|(>)\|(&quat)\|(&#39))/;
	const escapeTest = /(&(?:lt\|amp\|gt\|quat\|#39);)/;

feat: Enabled syntax highlight to a code block #83

feat: Enabled syntax highlight to a code block #83

Conversation

korosuke613 commented Jul 1, 2020 • edited Loading

korosuke613 commented Jul 1, 2020

korosuke613 commented Jul 9, 2020

ganta left a comment

Choose a reason for hiding this comment

korosuke613 commented Jul 12, 2020

ganta left a comment

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2020

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2020

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2020

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2020

Choose a reason for hiding this comment

korosuke613 commented Jul 14, 2020

ganta commented Jul 14, 2020

korosuke613 commented Jul 14, 2020

ganta left a comment

Choose a reason for hiding this comment

ganta Jul 16, 2020

Choose a reason for hiding this comment

korosuke613 Jul 16, 2020

Choose a reason for hiding this comment

ganta Jul 16, 2020

Choose a reason for hiding this comment

korosuke613 Jul 16, 2020

Choose a reason for hiding this comment

ganta Jul 17, 2020

Choose a reason for hiding this comment

korosuke613 Jul 17, 2020

Choose a reason for hiding this comment

ganta left a comment

Choose a reason for hiding this comment

korosuke613 commented Jul 1, 2020 •

edited

Loading