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 add member rewrite #48

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

Conversation

koedoyoshida
Copy link
Contributor

チケット

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

#35
の指摘反映書き直し版です。

@takanory takanory changed the title Issha 1329 add member rewrite ISSHA-1329 add member rewrite Feb 28, 2021
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.

コメントしました

@@ -235,6 +235,9 @@ BOT: @takanory おはようございます
- `$cal`: 今月のカレンダーを返す
- `$cal 9`: 今年の指定された月のカレンダーを返す
- `$cal 9 2016`: 指定された年月のカレンダーを返す
- `$members`: チャンネルにいる通常の参加者のメンション名の一覧を返す
Copy link
Member

Choose a reason for hiding this comment

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

MUST: membersは別プラグインなので、新しい見出し members plugin を追加して、そこに書いてください

import slacker
import json

from ..botmessage import botsend, botreply, botwebapi
Copy link
Member

Choose a reason for hiding this comment

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

SHOULD 使っていない import があるので削除してください

チャンネル参加者のメンション名の一覧を返す

- https://github.com/os/slacker
- https://api.slack.com/methods/conversations.members
Copy link
Member

Choose a reason for hiding this comment

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

channels.infoはdeprecatedになったので、APIを変更しました

https://api.slack.com/methods/channels.info

- https://github.com/os/slacker
- https://api.slack.com/methods/conversations.members
- https://api.slack.com/methods/users.list
- https://api.slack.com/methods/users.getPresence
Copy link
Member

Choose a reason for hiding this comment

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

SHOULD: このあたりのAPIも使ってなさそうなので削除してほしい

continue

# idが複数ヒットした場合警告を出す
if len(memberkeys) > 1:
Copy link
Member

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:
# 全ユーザー情報リストから該当するユーザで抽出
memberkeys = [x for x in all_user_info.body['members']
Copy link
Member

Choose a reason for hiding this comment

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

MAY: ループの外側で
[x for x in all_user_info.body['members'] if x['id'] in members] で一度にリスト作っちゃうんじゃだめかなーって思いました(動作確認はしてません)

display_name = memberkeys[0]['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.

SHOULD: if display_name だけでよい

# 探しやすいように大小文字区別なしアルファベット順
member_list.sort(key=str.lower)

pretext = 'このチャンネルの{0}参加者は以下{1}名です。'.format(desc, len(member_list))
Copy link
Member

Choose a reason for hiding this comment

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

MAY: f-stringに書き換えたい

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