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

「hh:mm」になったら投稿するremindコマンドを追加 #6

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

odaillyjp
Copy link
Member

#5

変更点

  • 「hh:mm」になったら投稿するremindコマンドを追加

急ぎで作ったので雑ですが、ひとまずidobataでテストしてみたいのでPRを送ります。致命的な問題がないかだけレビューしていただけますと嬉しいです。

ローカルでの確認方法

$ bin/hubot
charlotte > charlotte remind 18:00 名探偵コナン、この後すぐ!
Sat Sep 05 2015 18:00:00 GMT+0900 (JST) "名探偵コナン、この後すぐ!", accepted.

scheduleDate = new Date(currentYear, currentMonth, currentDay, hour, minute)
new scheduler.scheduleJob scheduleDate, =>
msg.send message
msg.send "#{scheduleDate} \"#{message}\", accepted."
Copy link
Contributor

Choose a reason for hiding this comment

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

acceptedsure とかにしたら上品さを醸し出すかもしれません。

Copy link
Member Author

Choose a reason for hiding this comment

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

上品な子にしたいですね! sure に変更します。

@yucao24hours yucao24hours deployed to yochi-charlotte-pr-6 September 5, 2015 09:37 Active
@yucao24hours yucao24hours deployed to yochi-charlotte-pr-6 September 5, 2015 09:52 Active
@yucao24hours
Copy link
Member

今さらだけど description の charlotte remind 18:00 名探偵コナン、この後すぐ! がナイスすぎます 🙈

@yucao24hours yucao24hours deployed to yochi-charlotte-pr-6 September 5, 2015 10:55 Active
@yucao24hours yucao24hours deployed to yochi-charlotte-pr-6 September 5, 2015 11:25 Active
@odaillyjp
Copy link
Member Author

2点修正しました。

  • リマインドを受け取ったときのメッセージの acceptedsure に変更
  • リマインドを受け取ったときのメッセージから日付を消す(今日のリマインドしかできないので、日付は不要かと思いました。)

@yucao24hours yucao24hours deployed to yochi-charlotte-pr-6 September 6, 2015 01:17 Active
@yucao24hours
Copy link
Member

この PR 外での対応でもいいと思うんですが、たとえば時間になったときに メンションをしたい!というとき

@Charlotte remind 10:30 @yucao24hours 神

ってやると、その時点で一度その人にメンションがいってしまって、ちょっと驚かせてしまうかも、という心配がでてきました 🐸

@odaillyjp
Copy link
Member Author

そうですね。例えば、remind とは別にメンション用のコマンドを用意するというのが無難な解決方法ですかねー。

@Charlotte remind to yucao24hors 10:30 神

@yucao24hours
Copy link
Member

例えば、remind とは別にメンション用のコマンドを用意する

それはよさそうですね 🌷
ただ、もう一度よく考えてみると特定の時刻に特定の人にメンションするっていうのは あまりないケースなのかなと思えてきました。
特定の時刻にメンションするケースとしてはおそらく アット allアット here という感じの、「その room にいる人たち == その room の話題に多かれ少なかれ関係している人たち」に対する発言になると思うのです(今考えた限りだと)。
そうしたいときは「時刻を渡すと、その時刻になったときに アット all でメンションをする」というコマンドを用意するといいのかもしれませんね( アット all という文字列はコマンドの中に直接書いておく)。

@yucao24hours
Copy link
Member

ということで、

  • この実装はこのまま
  • アット yucao24hours といった個別メンションの文字列を渡すかどうかはおまかせ(やるとしてもおそらく自分自身へのリマインドの意味でのメンションとかが多くなると思うので、あまり困らなさそう?)
  • そしてさらに、 アット allアット here のコマンドも別で実装。全員へのメンションはそっちを使ってもらう。

というのはいかがでしょうか!?

@yucao24hours yucao24hours deployed to yochi-charlotte-pr-6 September 9, 2015 03:53 Active
@5t111111
Copy link
Contributor

5t111111 commented Sep 9, 2015

個人的にはおだいさんの提案に 👍

理由は

  • all も here も 個別メンションもこれでいける
  • コマンドを別で実装すると覚えることが増えちゃう
  • remind [to 対象 : オプション] [時刻] [内容] とわかれているので文字列のパースもそんなに難しくない

という感じですかねー。

ただ欠点として

  • コマンド覚えなくていいけどタイポの危険性がある。remindの意図上これは避けたい。個別コマンド用意した方がこれは事前に気づけると思う
    @Charlotte remind to heree 10:30 神 -> @heree さんにのみメンション
  • 複数人を対象にできるようにするか?そのときどう書くか?などがたぶんでてくる。

とかがもあるかなとは思います。

あとは remind to someone というのは英語的には正しくないので、remind of someone とするかそのまま remind at someone とするか、いっそ remind someone とするかにした方がよいかもというのがあります。

@odaillyjp
Copy link
Member Author

返事が遅れてごめんなさい 🙇

allhere も個別メンションも同じコマンドでいきたいので、僕もできれば別コマンド(もしくはオプション)を用意したいと思っています。

ただ、このPRでは「ある時間になったら投稿する remind コマンドを追加する」だけにフォーカスが当たっているので、メンションについては別PRで対応させていただきたいです。

@yucao24hours
Copy link
Member

yucao24hours commented Apr 17, 2016

はっ。rebase needed!

@odaillyjp
Copy link
Member Author

忘れてました:bow: マージせねば

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.

4 participants