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

Feature/add subscribers #1

Merged
merged 9 commits into from Jan 25, 2019

Conversation

nagamoto
Copy link
Contributor

@nagamoto nagamoto commented Jan 24, 2019

PR1つに複数の変更項目を含めてみにくいかもしれませんが、もしよければ確認お願いします
何か問題あればばしばしご指摘くださいm(__)m

変更内容

  • HATENA_BLOG==="true"のときのみ、はてなブログ読者数を取得
  • typoらしきメソッド名を書き換え
  • appsscript.sample.jsonをGit管理下におく

@@ -0,0 +1,11 @@
{
"timeZone": "Asia/Tokyo",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

現状では動作に影響はないもののタイムゾーンをついでに東京に変更

"userSymbol": "Analytics",
"serviceId": "analytics",
"version": "v3"
}]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

analyticsを有効に設定

@budougumi0617 budougumi0617 self-requested a review January 24, 2019 08:12
Copy link
Owner

@budougumi0617 budougumi0617 left a comment

Choose a reason for hiding this comment

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

早速のPRありがとうございます!!!!
少しコメントしたのでご確認おねがいします。

.gitignore Outdated
@@ -61,4 +61,3 @@ typings/
.next

.clasp.json
src/appsscript.json
Copy link
Owner

Choose a reason for hiding this comment

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

これは必要な記述なので削除しないでほしいです。リポジトリには不要なファイルなので。
後述の通りappscript.jsonは配布できないんです。

@@ -0,0 +1,11 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

このファイルは削除してください。
理由は、appscript.jsonファイルはリポジトリに含めていてもclasp createしたときに初期化されるので、意味が無いからです。

$ clasp create --title "KPI Sheet" --type sheets --rootDir ./src
Created new Google Sheet: https://drive.google.com/open?id=...
Created new Google Sheets Add-on script: https://script.google.com/d/1.../edit
Cloned 1 file.
└─ ./src/appsscript.json

ここでappscript.jsonは初期化されます。

@@ -1,4 +1,4 @@
function getBookmarcCount(target) {
function getBookmarkCount(target) {
Copy link
Owner

Choose a reason for hiding this comment

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

ありがとうございます!!

src/main.ts Outdated
// BLOG_URLには自分のサイトのURLを入力しておくこと。
// 例: https://budougumi0617.github.io/
let numOfSubscribers = -1;
if (blogUrl != null) {
Copy link
Owner

Choose a reason for hiding this comment

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

はてなブログじゃない人はこのロジックはつかえないですよね?
はてなブックマーク数は取得したいけど、はてなブログじゃないひとはいると思います(というか私)。
なので、BLOG_URLが設定されていてかつString.prototype.includes()あたりではてなのURLをなのか絞る必要があると思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに全くその通りですね、考慮が足りてませんでした
はてなブログproの場合もあるはずなので、それも加味しつつ、正規表現など判定が難しそうであれば、別のpropertyを使うなど考えてみます

Copy link
Owner

Choose a reason for hiding this comment

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

簡単にオンオフ用のプロパティ追加でもいいと思います!
エラーが出ないならばdone better than perfectで!

}

console.log([today, followers, pv, bounceRate, bookmarks, numOfSubscribers]);
sheet.appendRow([today, followers, pv, bounceRate, bookmarks, numOfSubscribers]);
Copy link
Owner

Choose a reason for hiding this comment

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

余裕があればREADMEに読者数を取得する旨追加していただけると 🙏 (このへん)

このリポジトリを使ってApps Scriptとスプレットシートを作成すると、以下のKPI情報をシートに書き込んでいきます。  
未設定にしておけば取得もしないので、全てサービスからの情報が不要な方でも利用できます。 
後述するトリガーを設定しておくことで定期的にデータを集計できます。 


Twitter | フォロワー数
Google Analytics | 前日までの週間PV数、 前日までの週間直帰率
はてなブックマーク | はてなブックマーク総数

Copy link
Contributor Author

Choose a reason for hiding this comment

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

承知しました
コメントいただいた箇所以外にも、改善した後にREADMEに記載したほうがいいことがないか、再確認します

}]
},
"exceptionLogging": "STACKDRIVER"
}
Copy link
Contributor Author

@nagamoto nagamoto Jan 24, 2019

Choose a reason for hiding this comment

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

実際のappsscript.jsonをGit管理できないことは把握しました
似たように実際の設定ファイルをGit管理できないものは、.sampleをつけてGit管理下に置くような運用をすることが多いので、appsscript.sample.jsonを作成してみました
不要そうであれば削除するので言ってください

(必要であればREADMEに追記したほうが親切ですが、力尽きました笑)

Copy link
Owner

@budougumi0617 budougumi0617 left a comment

Choose a reason for hiding this comment

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

なんどもごめんなさい、suggest書きました!

README.md Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
budougumi0617 and others added 2 commits January 25, 2019 07:22
Co-Authored-By: nagamoto <nagamoto@users.noreply.github.com>
Co-Authored-By: nagamoto <nagamoto@users.noreply.github.com>
Copy link
Owner

@budougumi0617 budougumi0617 left a comment

Choose a reason for hiding this comment

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

LGTM!

@budougumi0617 budougumi0617 merged commit 8bf8974 into budougumi0617:master Jan 25, 2019
@nagamoto
Copy link
Contributor Author

丁寧にレビュいただいてありがとうございましたm(__)m

@nagamoto nagamoto deleted the feature/add_subscribers branch January 25, 2019 00:41
h-takeyeah added a commit to h-takeyeah/boushitsu-slackbot-relay that referenced this pull request Jan 22, 2021
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.

None yet

2 participants