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

ISSHA-1329 チャンネルにいる参加者の一覧を返すコマンドを追加 #35

Closed
wants to merge 5 commits into from

Conversation

koedoyoshida
Copy link
Contributor

@koedoyoshida koedoyoshida commented Feb 2, 2019

チケット

レビューしてほしいところ

  • 動作確認済み(#pyconjpbot-yoshida #2019)
  • READMEとChangeLog.txt記載しました。
  • コマンド名に突っ込みあれば。

Copy link
Member

@takanory takanory left a comment

Choose a reason for hiding this comment

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

レビューしました


# メンバ一覧から順次処理
for member_id in members:
user_info = webapi.users.info(member_id)
Copy link
Member

Choose a reason for hiding this comment

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

重要: この書き方だとmemberの数だけAPIを叩いてしまうので、ちょっと迷惑かなと
users.list APIを1回だけ呼び出して全ユーザー情報を取得し、その情報を使うのがよいかと

https://api.slack.com/methods/users.list

members = cinfo.body['channel']['members']

# 作業用リスト初期化
nameall = []
Copy link
Member

Choose a reason for hiding this comment

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

nits: 変数名がちょっとアレかなと... member_list とかでいいのでは

if display_name != "":
basename = display_name

if subcommand == 'all':
Copy link
Member

Choose a reason for hiding this comment

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

重要: 対象外だったらcontinueして、そうじゃなかったら最後に append するとコードが読みやすい

if subcommand == 'bot' and botじゃないユーザー:
    conitnue
if subcommand is None and botユーザー:
    continue
nameall.append(basename)

display_name = user_info.body['user']['profile']['display_name']

# display_nameが設定されていればそれが優先されている推測
if display_name != "":
Copy link
Member

Choose a reason for hiding this comment

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

nits: 以下でよい。空文字列でなければ true になる

if display_name:
    basename = display_name

nameall.sort(key=str.lower)

# 処理概要、一覧、Countを出力
botsend(message, 'このチャンネルの{0}参加者一覧は\n{1}\n{2}参加者です。'.format(
Copy link
Member

Choose a reason for hiding this comment

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

nits: メッセージとしては先に人数があった方が見やすいのでは。
あと、人数が多いと表示でメッセージが流れちゃうので、attachements使って折り畳まれて表示されるようにしたほうが良いかと。

https://api.slack.com/docs/message-attachments

このチャンネルの{}参加者はX名です。
takanory
hogehoge

@koedoyoshida
Copy link
Contributor Author

#48
で作り直したのでこれはClose

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