Update users channels and messages via API by thinca · Pull Request #102 · vim-jp/slacklog-generator

thinca · 2020-05-10T10:10:26Z

API 経由で users.json を更新する update-user-list サブコマンド
API 経由で channels.json を更新する update-channel-list サブコマンド
API 経由で指定した日付1日分の全チャンネルのログを取得してチャンネル毎の日付のファイルに保存する fetch-messages サブコマンド

を追加しました。

正直なところ設計とか命名とか全体的にイケてない感じがするので、アドバイスよろしくお願いします！

thinca · 2020-05-10T10:38:01Z

そう言えば messages に関しては並列化の余地がありますが

めちゃくちゃ遅いわけではない
50+ per minute の API rate limit がある
- 現状すでに 81 チャンネルあるので普通にやったら引っかかりそうだけど手元で試した時はひっかからなかった…よくわからない…
ぶっちゃけめんどい

ので保留にしました。

koron

DownloadEntitiesToFile は Go 的な書き方からはほど遠い。APIRequest にどのような値を設定すべきかそれを呼ぶ人が意識しなければいけない(さらにドキュメントもない)のは論外といってよい。この設計は書く手間を減らして読む手間を犠牲にしている感がある。

Go的に考えるなら以下のような構成になる。

まず conversations.history を正しく1ページ読める関数(メソッド)を作る
次にそれを1つのファイルに保存する仕組みを作る
それを必要な回数イテレーション(=ページネーション)する仕組みを作る
必要なら複数ファイルに渡ったデータを1つのファイルにまとめる仕組みを作る
このとき既存のgeneratorの入力フォーマットも再考してよい

conversations.list や users.list は多少書く手間がかかっても別の入り口になってるべき。
DownloadConversationHistory, DownloadConversationList, DownloadUsersList みたいに。
それぞれ最小限の引数を受け取るのが好ましい。

いっぺんに3つ作るのではなく1つを正しく作る。それを3回繰り返す。そのうえでまとめられるところはまとめる、というのが正着ではないでしょうか。

以上、かなり辛目なレビューでした。

koron · 2020-05-10T10:16:38Z

+	"path/filepath"
+)
+
+type APIRequest struct {


公開型はかならずコメントを付けましょう。

golint 入れておいて make lint で警告がでるんで
それが増えないようにしてください。

koron · 2020-05-10T10:18:05Z

+}
+
+func reverse(slice []interface{}) {
+	for i := 0; i < len(slice)/2; i++ {


毎回 len()/2 はあんま良くないですね。

こんな感じでどうでしょか?

Suggested change

for i := 0; i < len(slice)/2; i++ {

for i, j := 0, len(slice)-1; i < j; i++, j-- {

koron · 2020-05-10T10:18:29Z

+	}
+}
+
+func DownloadEntitiesToFile(request APIRequest, destJSONFilePath string) error {


公開関数は…以下略

koron · 2020-05-10T10:24:01Z

+	if !ok {
+		return ""
+	}
+	metadata := metadataValue.(map[string]interface{})


キャスト(type assertion)に失敗したケースについて考慮が抜けてます。

他にも何か所かあるみたいですが指摘は省略しますので留意してください。

Suggested change

metadata := metadataValue.(map[string]interface{})

metadata, ok := metadataValue.(map[string]interface{})

if !ok {

return ""

}

koron · 2020-05-10T10:26:02Z

+	for true {
+		var json map[string]interface{}
+
+		fmt.Printf("GET %v page %d of %s ... ", request.ExtraParams, pageNum, request.APIMethod)


ログは log パッケージ使ったほうが良いです。

Suggested change

fmt.Printf("GET %v page %d of %s ... ", request.ExtraParams, pageNum, request.APIMethod)

log.Printf("GET %v page %d of %s ... ", request.ExtraParams, pageNum, request.APIMethod)

これ、今のところ場所によってまちまちなので統一したいですね > log or fmt
場所によっては fmt.Fprintln() で os.Stderr に出してるとこもあります

そうなんですよね。ほんとは脳死で log.Printf 使っておいてほしいんです。
そうすれば後から logging ライブラリを別のやつに差し替えるとかも簡単なんで。

koron · 2020-05-10T11:19:05Z

+		list := json[request.JSONKey].([]interface{})
+		fmt.Printf("fetched %s count: %d\n", request.JSONKey, len(list))
+
+		results = append(results, list...)


patination が必要だと考えてるものを、メモリ上で全部合成するのはあんま嬉しくなさげ。
以下みたいに都度ファイルに出力してしまったほうが良いのでは?

もしくはドキュメントコメントにあんま大きなものは取らないで、みたいに書くべきか?

[ {"entity": "123456"}, {"entity": "123456"}, {"entity": "123456"}, {"entity": "123456"}, {"entity": "123456"}, {"entity": "123456"}, ]

reverse との兼ね合いはあるが…そもそもJSONに書き出す段階で reverse する=順序を気にする必要ってなんなんだろう?

どうせ読み込む時は全部メモリに読みますが…しかも全期間分のものを同時に。ダウンロード時にだけそこを気にする必要ありますかね?

いまここから手を付けておけばその方向性が示せますよね。
generatorも今の全部メモリに乗せる方式は破綻が見えてるので、直さなきゃいけないですから。

downloaderは SQLiteとかのローカルのDBに逐次乗せる。
generatorはそれを切り出して1ページずつHTML生成
みたいなのが目指すべきゴールかなと。

generator 側はともかく、こちらは1チャンネル/1日分です。1チャンネルの1日分のログのサイズはたかが知れていると思っているんですが(実際に 400 件以上の発言があった場合でも JSON で 300KB に満たない、メモリに展開しても 1MB も取ることはなさそう)、そこを今頑張る必要あるでしょうか?
この部分は過去の全ログを処理する generator と違って、常に1日分なので今後ログが蓄積していっても処理する量が増えて行ったりはしません。
メモリも基本的に潤沢な今の時代にそこを気にするのは早すぎる最適化を感じてしまいます。

koron · 2020-05-10T11:23:16Z

+			return nil, fmt.Errorf("Error response: %v", json)
+		}
+
+		list := json[request.JSONKey].([]interface{})


キーがみつからないケース。型アサーションに失敗したケースの考慮もれ。

koron · 2020-05-10T11:31:32Z

+type APIRequest struct {
+	SlackToken  string
+	APIMethod   string
+	ExtraParams map[string]string
+	JSONKey     string
+	Reverse     bool
+}


名前と設計があってない。

APIRequest だと将来的にすべてのAPIを叩けるようになりそうで、そのパラメータをすべて内包しそう。
slackclient.Do(APIRequest) みたいな使い方が想定される。

また DownloadEntitiesToFile() (Pagination) 専用だとしても必須パラメータとオプショナルがまざってて、使う法としては困惑する。

仮に自分がやるならこうなる

type DownloadEntitiesOptions struct { ExtraParams map[string]string Reverse bool } func DownloadEntitiesToFile(token, method, itemKey, dstName string, opts DownloadEntitiesOptions) error {

もしくは token は必須だとしてメソッド生やす形かな。

type SlackClient struct { token string c *http.Client } type DownloadEntitiesOptions struct { ExtraParams map[string]string Reverse bool } func (sc *StackClient) DownloadEntitiesToFile(method, itemKey, dstName string, opts DownloadEntitiesOptions) error {

いい名前が思い付かなくて諦めたやつです…。
愚直に引数を10個でも20個でも並べた方がいい感じなんですね。構造体使うのやめてみます。

必須パラメータはなるべく引数に倒したほうが使うときにわかりやすいです。
で省略しても問題ないオプショナルパラメーターは構造体に突っ込むと。

もしくは全部の引数を構造体に入れるなら、全フィールドに
「これは必須」
「こっちはオプショナルでこういう時に使ってね」
みたいなドキュメント書くと良いです。

koron · 2020-05-10T11:39:40Z

+	return nextCursorValue.(string)
+}
+
+func httpGetJSON(rawurl string, params map[string]string, dst interface{}) error {


APIの汎用性を考えると params は map[string][]string のが良いかもしれない。
同じ名前の query string ってなんだよって思うけれど url.Values はそれを考慮した設計になってるし、
まれにそういうAPIもある。

当然 values.Set ではなくて values.Add で地道に構築する形になる。

そのケースは使わないだろうと判断して省略しました。

koron · 2020-05-10T11:43:09Z

+				"limit":   "200",
+			},
+			JSONKey: "messages",
+			Reverse: true,


この順序を保証しなきゃいけないのは、実は generator の処理の仕方に問題があるからで、それをココで吸収するのは好ましくない。

本 PR とは独立した問題ですね。Issue 立てておいてもらえると…。 🙏

#109

説明はあとから

kuuote · 2020-05-10T12:03:46Z

+	var nextCursor string
+
+	pageNum := 1
+	for true {


ここ、for { でよかったりします

tennashi · 2020-05-10T13:48:42Z

個人的には Downloader に寄せれそうに思いますが、それは後でやればいいので、どこでどの処理を担保するのかが定まってれば良いと思っています // というお気持ちだけ忘れないよう書いておきます

thinca

レビューありがとうございます。勉強になります。

まず conversations.history を正しく1ページ読める関数(メソッド)を作る
次にそれを1つのファイルに保存する仕組みを作る

これをやってしまうと1日1ファイルで保存すると言う目的が達成できなくなってします。

全体としてどう直すのがいいのか、指摘からだとちょっとわからず…。
とりあえず細かい指摘の部分を直してみます。

thinca · 2020-05-10T16:30:33Z

+		list := json[request.JSONKey].([]interface{})
+		fmt.Printf("fetched %s count: %d\n", request.JSONKey, len(list))
+
+		results = append(results, list...)


どうせ読み込む時は全部メモリに読みますが…しかも全期間分のものを同時に。ダウンロード時にだけそこを気にする必要ありますかね?

thinca · 2020-05-10T16:31:17Z

+	return nextCursorValue.(string)
+}
+
+func httpGetJSON(rawurl string, params map[string]string, dst interface{}) error {


そのケースは使わないだろうと判断して省略しました。

thinca · 2020-05-10T16:33:04Z

+				"limit":   "200",
+			},
+			JSONKey: "messages",
+			Reverse: true,


本 PR とは独立した問題ですね。Issue 立てておいてもらえると…。 🙏

thinca · 2020-05-10T16:41:50Z

+type APIRequest struct {
+	SlackToken  string
+	APIMethod   string
+	ExtraParams map[string]string
+	JSONKey     string
+	Reverse     bool
+}


いい名前が思い付かなくて諦めたやつです…。
愚直に引数を10個でも20個でも並べた方がいい感じなんですね。構造体使うのやめてみます。

thinca · 2020-05-11T02:19:53Z

#116 を見ましたが、理解しづらく、今の私ではレベルが足りずに足を引っ張りそうなのでこの件からは手を引きます。お手数をおかけしますが後は任せました。

koron requested changes May 10, 2020

View reviewed changes

kuuote reviewed May 10, 2020

View reviewed changes

thinca commented May 10, 2020

View reviewed changes

thinca force-pushed the update-users-channels-and-messages-via-api branch from a1f3b12 to 84780c6 Compare May 10, 2020 19:14

thinca added 4 commits May 11, 2020 04:22

Add api.go to support API request

a7cd103

Add update-user-list subcmd

610da6d

Add update-channel-list subcmd

dea4276

Add fetch-messages subcmd

ea295a7

thinca force-pushed the update-users-channels-and-messages-via-api branch from 84780c6 to ea295a7 Compare May 10, 2020 19:22

thinca closed this May 11, 2020

	for i := 0; i < len(slice)/2; i++ {
	for i, j := 0, len(slice)-1; i < j; i++, j-- {

	fmt.Printf("GET %v page %d of %s ... ", request.ExtraParams, pageNum, request.APIMethod)
	log.Printf("GET %v page %d of %s ... ", request.ExtraParams, pageNum, request.APIMethod)

Uh oh!

Conversation

thinca commented May 10, 2020

Uh oh!

thinca commented May 10, 2020

Uh oh!

koron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tennashi commented May 10, 2020

Uh oh!

thinca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thinca commented May 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants