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

feat: input parser #115

Merged
merged 13 commits into from
Jan 20, 2020
Merged

Conversation

yumetodo
Copy link
Contributor

@yumetodo yumetodo commented Jan 17, 2020

入力をパースする関数を追加しました。

実装にあたり、単体テストなしで書くのは私が死んでしまうので、勝手ながら単体テストを導入しました。 c11b5f3 のコミットメッセージをご確認ください。

https://circleci.com/docs/2.0/collect-test-data/#ava
を参考にcircle ciでテスト結果を認識できるようにしてあります。

なお、念の為ですがこのPRは @yume-yu さんの要請に応じた、 #37 のための準備、という位置づけです。

ref

新機能実装にあたって、さすがにテストコードもなしに実装するのは辛さがある。テストを書きたい。

Node.jsによる ES Moduleの対応はv13でようやく使えるようになりつつあるがあくまで実験段階である。
https://qiita.com/shimataro999/items/8a63ec06f33ccd2ea9ca

そんななかでテストフレームワークを選定するなかで、まずjestを試した。普段私が使っているからだ。

ところがjestはES Moduleに対応していない。
ts-jest経由でtypescriptで書くとか、
babelに掛けるとかすれば行けそうではあったが設定がだるい。

そこでavaを試した。
https://github.com/avajs/ava/blob/master/docs/recipes/es-modules.md
によれば、avaもまた現在ES Moduleに対応していないが、
esmというパッケージと容易に組み合わせることができ、これによってテストが実行できるようだ。
しかも現在のNode.js LTSである12系でも動く。
まあesmも結局babel呼び出しているのだが、設定が容易なのだからまあいいだろう。

したがってこれを採用した。
@yumetodo yumetodo requested a review from yume-yu January 17, 2020 04:31
@yumetodo yumetodo marked this pull request as ready for review January 17, 2020 04:32
Copy link
Contributor

@sasanquaneuf sasanquaneuf left a comment

Choose a reason for hiding this comment

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

致命的ではないと思いますがコメントしました

test/common.test.js Show resolved Hide resolved
test("inputParser", t => {
// Toot ID

t.notThrows(() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

全体的にテストで戻り値見てないですね(inputParserのコメント参照)
関数呼び出しまでのテストになっています、もともとonPURLとかの結果を返すつもりがなければそれでもよいような気はします。

* @param {(tootId: string)=>void} onToot 入力がTooT URLかToot IDだったときのcallback
* @returns {void} callbackが返した値
*/
export function inputParser(input, onPURL, onToot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

これ設計的な話ですけど、onPURLとonTootを渡した方がいいですか?
引数をinputだけにして、戻り値をstring[]だけにする(tootIdだけの場合もarrayにしちゃう)か、戻り値にPURLかTootかの区分を持たせるようにして、callbackを渡さないようにしたほうが、色々処理しやすそうな気はしました。
(強制したいということではないので、判断に従います)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

処理フローが違うので、戻り値で色々やるのは二度手間かなと思ったりしてました。

js/common.js Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
@blhsrwznrghfzpr blhsrwznrghfzpr added the enhancement New feature or request label Jan 17, 2020
@yumetodo
Copy link
Contributor Author

なんか時折deploy-preview がgithub上では終わってないようになるの何なんでしょうね。

test/common.test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@blhsrwznrghfzpr blhsrwznrghfzpr left a comment

Choose a reason for hiding this comment

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

LGTM

@sasanquaneuf
Copy link
Contributor

LGTM
※コメントしていたのでいちおう

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants