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

[Add] TODOリクエスト機能の追加 #3

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
2 changes: 1 addition & 1 deletion cogs/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ async def do_eval(self, ctx, body):
value = stdout.getvalue()
embed = discord.Embed(
title="Error",
descriription=f"エラーが発生しました。コードを確認してください。\ntraceback:```py\n{value}{traceback.format_exc()}\n```"
description=f"エラーが発生しました。コードを確認してください。\ntraceback:```py\n{value}{traceback.format_exc()}\n```"
)
await ctx.send(embed=embed)
else:
Expand Down
85 changes: 83 additions & 2 deletions cogs/todo.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,25 @@
import discord
from discord.ext import commands
import json
import hashlib


class TODOCog(commands.Cog):
def __init__(self, bot):
self.bot = bot

def make_request(self, ctx, to_user, todo):
Copy link

Choose a reason for hiding this comment

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

to_user 、「to」な「user」って意味なんだろうけど、「ユーザーに向けて」みたいに読めちゃうので混乱しちゃうな。
俺だったら user_send_to とかかな。まあ俺の案もそんなにイケてる感じしなくて微妙なラインなので、別に修正しなくてもまあいいかって感じではある。

Copy link

Choose a reason for hiding this comment

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

見てる感じ内部用メソッドっぽいので、メソッド名は _ つけてあげるのが丁寧かな。

Copy link

Choose a reason for hiding this comment

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

オブジェクト指向のコツの一つは、データごとにロジックを整理すること。
例えば今回の場合、リクエストに関するロジックを Request というクラスやモジュールにまとめるのが自然。

Cog はあくまで Discord とのやり取りを表現するクラス。
こういう IO 関係のロジックと、IO と関係ないプレーンなロジックを分離できるようになると、コードの見通しはすごいよくなる。
「Discord とのやりとりでエラーが起きてるようなら、Cog をチェックすればいい。」
「リクエストの保存の仕方でミスってるようなら、Request をチェックすればいい。」
という感じ。
あと、IO を切り離すとテストコードでテストすることも可能になったりする。

ちなみに、save_data も一つの IO といえる。この辺のロジックも分離できると更にスッキリするしテストしやすくなる。
俺だったら RequestRepository みたいなクラス作るかな。「リポジトリパターン」っていうデザインパターンがあるのでそれを使う。詳しくはググれ。

まあ、このへんは設計レベルのリファクタリングであって、重いタスクかつ発展課題なので、「やらなきゃマージできない〜〜〜」とか思わなくていいよ。「ほーん、そうなんや。まあとりあえずマージしちゃお。」でいい。

Copy link
Owner Author

Choose a reason for hiding this comment

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

分離ですか...ちょっとすぐにはできそうもないので、今は今後の課題、という形で処理させていただこうと思います。
教えていただきありがとうございます!

text = f"{ctx.message.id} - {ctx.author.id} -> {to_user.id}"
hashed = hashlib.sha256(text.encode()).hexdigest()
data = {
"author": ctx.author.id,
"to": to_user.id,
"content": todo
}
self.bot.data["request"][hashed[:10]] = data
Copy link

Choose a reason for hiding this comment

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

sha256 の前10文字だけ取ってくるって被らないのかな?
あと hashed[:10] を下でも使っているので、request_id 的な変数を定義して代入すると分かりやすいと思う

Copy link

Choose a reason for hiding this comment

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

あと hashed[:10] を下でも使っているので、request_id 的な変数を定義して代入すると分かりやすいと思う

分かりやすいだけでなくで、修正漏れも防げる。いわゆるDRY原則というやつ。

Copy link

Choose a reason for hiding this comment

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

てか、ただ一意な ID 振りたいだけなら、UUID とか使えばいい気がする。

Copy link
Owner Author

@daima3629 daima3629 Jul 14, 2020

Choose a reason for hiding this comment

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

このIDを入力してリクエストの承認・拒否を行うので、長すぎると手入力がとても大変になるんですよね...
先程shunさんから「36進数に圧縮すればいい」という意見を頂いたので、そうするように修正コミットを出しました!
624a42cこちらです!

Copy link

Choose a reason for hiding this comment

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

このIDを入力してリクエストの承認・拒否を行うので、長すぎると手入力がとても大変になるんですよね...

大体ダブルクリックして ctrl+C だろうけどね。

てか、いつの間にかメッセージ ID をリクエストの ID にするようになってる。かしこ。

shunさんから「36進数に圧縮すればいい」という意見を頂いた

[0-9a-z] で 36 ってわけか。かしこすぎる。

Copy link

Choose a reason for hiding this comment

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

dict 使ってるのは JSON との相互変換しやすいからなんだろうけど、dict でこういうデータ扱うと色々メンテ大変だよ。

e.g.

  • クラスと違って、データの仕様(どんなプロパティがあるか)がどこかにまとまってるわけではない。
  • スペルミスしてもエラーが出てくれない。

クラスで作ったオブジェクトでも JSON と相互変換はできる。
ちょっと手間かかるので、あくまでリファクタリング課題って感じだけど、ちゃんとクラス定義してみるといいと思う。(ちょっと重いタスクかもしれないので、今はやらずに課題としてどこかに残しておく程度でも可。)

self.bot.save_data()
return hashed[:10]
Copy link

@ikngtty ikngtty Jul 14, 2020

Choose a reason for hiding this comment

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

メソッド名が make_request だけど、内容は

  1. リクエストのデータを作成
  2. 作成したデータを bot オブジェクト内に格納
  3. 作成したデータを外部ファイルに保存(bot.save_data の呼び出し)
  4. 作成したリクエストの ID を返す

の4つだね。

問題点:

  1. 一つのメソッドが複数のことをやっている。単一責任原則に反している。
    • 一度に複数のことを実現しようとすると、人は混乱するし、バグを生みがち。
    • その他詳しくは「単一責任原則」でググれ。
  2. 名前が実体を表していない(内容 1 は合っているが、内容 2 〜 4 は名前に表れていない)。
    • 「メソッド名を見ただけで何をしているかが分かる」という状況が理想。そういう状況だと、脳に負担をかけずに読むことができる。今の状況だと、make_request を使っている部分を読む時に、「ええと、make_request は本当はリクエストの保存をしているから…」という感じで、make_request の中身まで頭に叩き込まないと読めないようになってしまっている。こういう状況では全体を俯瞰してチェックしたりするのが困難になる。

というわけで、内容に沿ってちゃんと分割して、それぞれに内容に即したメソッド名をつけることをオススメする。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ここらへんはすぐにはできそうではないので、今後の課題とさせていただきたいと思います、ご指摘ありがとうございます!


@commands.command(name="help")
async def _help(self, ctx):
msg = """
Expand Down Expand Up @@ -47,13 +60,14 @@ async def on_message(self, message):
msg = await message.channel.send(msg)
Copy link

Choose a reason for hiding this comment

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

message(受信したメッセージのオブジェクト)
msg(確認のために送信するメッセージのテキスト)
再代入して
msg(確認のために送信したメッセージのオブジェクト)
ってなんか凄く追いづらい気がする

変数名を
confirm_message_text
confirm_message
とかにすると分かりやすい

await msg.add_reaction("👍")

Choose a reason for hiding this comment

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

突然失礼します
レビュー見ました。
UTF-8のアイコンを使用するのは良いのですが、個人的に👍👍🏻など亜種も多数存在するため、定数を切ると良いと思いました

ICON_GOOD = "👍"

的な・・・?

Copy link
Owner Author

Choose a reason for hiding this comment

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

確かに定数を使ったほうがいいですね、ご指摘ありがとうございます!
反映させていただきます!

await msg.add_reaction("👎")

def check(reac, user):
if not user == message.author:
return False
if not str(reac.emoji) in ["👍", "👎"]:
return False
return True

reac, _ = await self.bot.wait_for("reaction_add", check=check)
if str(reac.emoji) == "👍":
if not self.bot.data.get(str(message.author.id)):
Expand Down Expand Up @@ -92,14 +106,81 @@ async def delete(self, ctx, num: int):
return await ctx.send("> そのTODOはありません。`todo!list`で確認してください。")
self.bot.save_data()
return await ctx.send(f"> TODO`{deleted_todo}`の削除に成功しました。")

@delete.error
async def error_delete(self, ctx, err):
if isinstance(err, commands.errors.BadArgument):
return await ctx.send("> 引数は整数で指定してください。")
if isinstance(err, commands.errors.MissingRequiredArgument):
return await ctx.send(">>> TODOの番号を引数で指定してください。\n番号は`todo!list`で確認できます。")

@commands.command()
async def request(self, ctx, member: discord.Member, *, todo):
text = f"""
>>> `{str(member)}`さんにtodoリクエストを送ります。
Copy link

Choose a reason for hiding this comment

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

なんで全体的に引用メッセージ(>>>がついてるやつ)なの?

Copy link
Owner Author

Choose a reason for hiding this comment

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

返信遅くなって申し訳ないです
そうしないと一行ずつ>をつけなくてはいけないので
現在のデザインとして、BOTのメッセージには全て引用符をつけるような感じになってます
でもやはりembedのほうが見やすいと思うので、今後デザインは一新しようと思ってます

Copy link

Choose a reason for hiding this comment

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

あ、今思いついたけど、いちいち引用にするのってどこかで忘れそうなので、

async def send_bot_message(ctx, message):
    await ctx.send(f">>> `{message}`)

みたいな util 関数作ってもいいかもねぇ。
embed に切り替えるのも一発でできるようになるかも。

ただ、なまじ共通化しすぎても柔軟性に欠けるから危険なんだよね。例外が色々出てきたりするとまた困ったりする。例外がどれだけ出てきそうかの将来予測次第かな。

内容:
・{todo}

よろしいですか?"""
msg = await ctx.send(text)
await msg.add_reaction("👍")
await msg.add_reaction("👎")

def check(reac, user):
Copy link

Choose a reason for hiding this comment

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

reac みたいに無理やり略すのは個人的には奨励しない。
多分一般的にもあんまやらないんじゃないかなー、まあでも Python 界隈はこういう略し方好みそうな気もするし、ちょっと自信ないや。

一番の理由として、脳内で読み辛いんだよね。
「リアクション ドット メッセージ イコール〜」って読みたい。
「リアク ドット メッセージ イコール〜」だと頭に入らない。

まあ個人的意見なので採用するかは自由。

Copy link
Owner Author

Choose a reason for hiding this comment

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

discord.pyのリファレンスのコード例の中でそんな略し方をしていた気がしたのでそうしていたんですが、今見たら普通にreactionで書いてありました...
ありがとうございます!

if not user == ctx.author: return
if not reac.message == msg: return
if not str(reac.emoji) in ["👍", "👎"]: return
return True

reac, _ = await self.bot.wait_for("reaction_add", check=check)
if str(reac.emoji) == "👎":
await msg.delete()
return await ctx.send("> todoリクエストをキャンセルしました。", delete_after=5)

req_id = self.make_request(ctx, member, todo)
dm_msg = f"""
>>> {ctx.author.mention}さんからTODOリクエストが届きました。

内容:
・{todo}

リクエストID: `{req_id}`

このリクエストを承認する場合は`todo!request_approve {req_id}`
拒否する場合は`todo!request_deny {req_id}`
とコマンドを実行してください。"""
Copy link

Choose a reason for hiding this comment

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

Python の 複数行文字列リテラルってインデント考慮してくれないんだ。
めちゃキモいな。
Python 界隈的には普通なのかもしれないけど、個人的には無理矢理にでも避けたいなこれ。

https://stackoverflow.com/a/33924077/12964914
というわけでちょっと探したけど、↑これぐらいしかワークアラウンド見つからんかった。

俺だったらこのワークアラウンド採用するかな。
まああくまで個人的価値観なので、採用するかは任せた。

Copy link
Owner Author

Choose a reason for hiding this comment

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

これに関してはtextwrap.dedentという関数を使うことでインデントを治すのが一般的なようです。
あまり気にしていませんでしたが、暇があれば修正します、指摘ありがとうございます!

Copy link

Choose a reason for hiding this comment

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

textwrap.dedentという関数を使うことでインデントを治すのが一般的

そうなんだ。なるへそ。じゃあそれで

try:
await member.send(dm_msg)
except:
Copy link

Choose a reason for hiding this comment

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

全ての Exception を握りつぶすんじゃなく、起こりうる Exception のみキャッチすると良いと思う。

await ctx.send(f"{member.mention}\n" + dm_msg)

result = f"""
>>> リクエストの送信に成功しました。

リクエストID: `{req_id}`"""
return await ctx.send(result)

@commands.command()
async def request_approve(self, ctx, req_id: str):
req_todo = self.bot.data["request"].get(req_id)
if not req_id:
Copy link

Choose a reason for hiding this comment

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

Suggested change
if not req_id:
if not req_todo:

ミス?

Copy link

Choose a reason for hiding this comment

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

deny の方と統一されてないので、何が正しいかわからなくなった

return await ctx.send("> そのIDのリクエストは存在しません。もう一度確認してください。")

self.bot.data["todo"][str(ctx.author.id)].append(req_todo.content)
del self.bot.data["request"][req_id]
self.bot.save_data()
return await ctx.send(f"> リクエストを承認しました。\n\n追加されたTODO:\n・{req_todo.content}")

@commands.command()
async def request_deny(self, ctx, req_id: str):
req_todo = self.bot.data["request"].get(req_id)
if not req_id:
return await ctx.send("> そのIDのリクエストは存在しません。もう一度確認してください。")

del self.bot.data["request"][req_id]
self.bot.save_data()
return await ctx.send(f"> リクエストを拒否しました。")
Copy link

@chun37 chun37 Jul 13, 2020

Choose a reason for hiding this comment

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

Suggested change
return await ctx.send(f"> リクエストを拒否しました。")
return await ctx.send("> リクエストを拒否しました。")

無意味な f-strings を発見!



def setup(bot):
bot.add_cog(TODOCog(bot))
Expand Down