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

Implement fetch-messages #138

Merged
merged 6 commits into from
Jul 6, 2020
Merged

Implement fetch-messages #138

merged 6 commits into from
Jul 6, 2020

Conversation

kyoh86
Copy link
Contributor

@kyoh86 kyoh86 commented Jul 6, 2020

概要

KoRoNさんが途中まで書き上げていたfetch-messagesのPR

#116

を引き継ぐ形で実装してみました。

私の理解 = やや不安なところ

internal/slackadapterの実装方針の理解があっているかどうかに少し不安が。
(動くのでリファクタリングレベルでの話である)

  • 責務は「使いやすい param / response で公式ライブラリをラップ」
    • 使いにくい定義のWrap
      • timestamp string <=> *time.Time
      • Cursor 処理の隠蔽
      • やや冗長な書き味の箇所が含まれるので、もう少しSmartな定義にしたかったかも
        • internal/slackadapter/conversations_history.go#L33-38とか
    • クライアントの生成も各関数の中で実施する
      • slack.Newを裸でそのまま各関数に書かせるのは少しリスクかも
      • NewしたClientを持ち回すのとどっちが良いかは見えていない

試し方

$ go run . fetch-messages --data-dir ./_logdata/slacklog_data --date 2020-07-05 --verbose

@kyoh86 kyoh86 marked this pull request as ready for review July 6, 2020 15:24
Copy link
Member

@koron koron left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@koron koron merged commit 02dcecc into vim-jp:master Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants