Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: nodecli #937

Merged
merged 9 commits into from
Aug 19, 2019
Merged

refactor: nodecli #937

merged 9 commits into from
Aug 19, 2019

Conversation

lacolaco
Copy link
Collaborator

@bot-user
Copy link

bot-user commented Aug 11, 2019

Deploy preview for js-primer ready!

Built with commit 52032c6

https://deploy-preview-937--js-primer.netlify.com

@lacolaco lacolaco requested a review from azu August 11, 2019 05:52
@@ -27,7 +27,7 @@ CommonJSモジュールからオブジェクトをエクスポートするには
`require`関数を使い指定したファイルパスのJavaScriptファイルをモジュールとしてインポートできます。
次のコードでは先ほどの`greet.js`のパスを指定してモジュールとしてインポートして、エクスポートされた関数を取得しています。

[import, title:"greet-index.js"](src/example/greet-index.js)
[import, title:"greet-main.js"](src/example/greet-main.js)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ブラウザの文脈じゃないからindexよりmainのほうがエントリポイントのニュアンスは出るかと思いました

@azu
Copy link
Collaborator

azu commented Aug 13, 2019

program.argsprocess.argv が紛らわしいから、

const program = require("commander"); const cli = require("commander"); とかでもよさそうとか思ったけど、ドキュメントが https://github.com/tj/commander.js programなんだな

Copy link
Collaborator

@azu azu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

source/use-case/nodecli/argument-parse/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@azu azu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

細かいtypoとかまだちゃんと見てないけど、全体的にスムーズに読みやすくなったと思う。

ユニットテストの所、ユニットテストとはなに?という説明がなかったのと

ユニットテストを行うためにはテスト対象がモジュールとして分割されていなければいけません

は本当? というのがちょっとだけひっかかったかな

source/use-case/nodecli/md-to-html/README.md Outdated Show resolved Hide resolved
@@ -37,12 +37,12 @@ CommonJSモジュールからオブジェクトをエクスポートするには
このようにエクスポートされたオブジェクトは、`require`関数の戻り値であるオブジェクトのプロパティとしてアクセスできます。
次のコードでは先ほどの`functions.js`をインポートして取得したオブジェクトから`foo`と`bar`関数をプロパティとして取得しています。

[import, title:"functions-index.js"](src/example/functions-index.js)
[import, title:"functions-main.js"](src/example/functions-main.js)

## アプリケーションをモジュールに分割する {#split-script}

それではCLIアプリケーションのソースコードをモジュールに分割してみましょう。
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
それではCLIアプリケーションのソースコードをモジュールに分割してみましょう
それではCLIアプリケーションのソースコードをCommonJSのモジュールに分割してみましょう

うーんどっちでもいいか


## アプリケーションをモジュールに分割する {#split-script}

それではCLIアプリケーションのソースコードをモジュールに分割してみましょう。
`md2html.js`という名前のJavaScriptファイルを作成し、次のようにMarkdownの変換処理を記述します
`md2html.js`という名前のJavaScriptファイルを作成し、次のようにmarkedを使ったMarkdownの変換処理を記述します

[import title:"md2html.js](./src/md2html.js)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[import title:"md2html.js](./src/md2html.js)
[import title:"md2html.js"](./src/md2html.js)

それぞれの変換オプションについて、コマンドライン引数で制御できるようにします。
`gfm`オプションは`--gfm`、`sanitize`オプションは`--sanitize`と`-S`でコマンドラインから設定できるようにします。
コマンドライン引数で`--gfm`のようなオプションを扱いたいときには、commanderの`option`メソッドを使います。
次のように必要なオプションを定義してからコマンドライン引数をパースすると、`program.opts`メソッドでパース結果のオブジェクトを取得できます。

<!-- 差分コードなので -->
<!-- doctest:disable -->
```js
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<div class="code-filename-block">
    <p class="code-filename">main.js</p>
</div>

#927 でインラインに対してもタイトルをつけられるようにしたい

Copy link
Collaborator

@azu azu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントした所以外はOKそう

source/use-case/nodecli/md-to-html/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@azu azu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントした所以外はOKそう

Co-Authored-By: azu <azu@users.noreply.github.com>
@lacolaco
Copy link
Collaborator Author

mergeします

@lacolaco
Copy link
Collaborator Author

#927 の件はまたあとで対応ということで。

@lacolaco lacolaco merged commit 7227702 into master Aug 19, 2019
@azu azu deleted the refactor-nodecli branch August 26, 2019 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants