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

daima3629 · 2020-07-13T14:33:19Z

色々間違えたのでやり直しPRです

Command request
- 第1引数にTODOリクエストを送りたいユーザーのメンション or ID or 名前を指定
- 第2引数(可変)に送りたいTODO文字列を指定
- リクエストは送り先のユーザーのDMに送られる
- リクエストごとにIDが作成される(sha256)
- IDを使い後述のコマンドでリクエストの承認, 拒否を行う
Command request_approve
第1引数にリクエストIDを指定
指定されたIDに対応するTODOリクエストを承認, 自身のTODOリストに追加する
Command request_deny
- 第1引数にリクエストIDを指定
- 指定されたIDに対応するTODOリクエストを拒否

※リクエスト承認, 拒否はリクエストをしたユーザーには通知されない, 今後通知されるよう改善する可能性あり

chun37 · 2020-07-13T16:46:46Z

cogs/todo.py

+            "to": to_user.id,
+            "content": todo
+        }
+        self.bot.data["request"][hashed[:10]] = data


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

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

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

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

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

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

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

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

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

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

chun37 · 2020-07-13T16:53:51Z

cogs/todo.py

+とコマンドを実行してください。"""
+        try:
+            await member.send(dm_msg)
+        except:


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

chun37 · 2020-07-13T16:56:29Z

cogs/todo.py

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


Suggested change

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

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

無意味な f-strings を発見！

chun37 · 2020-07-13T17:04:49Z

request_approve と request_deny の共通してるところが多いので、共通してる部分を関数化すると良いかも

commands.group() を使って、request create, request list, request approve, request denyみたいにすると、個人的に分かりやすいと思う。

daima3629 · 2020-07-13T22:50:25Z

たくさんの指摘ありがとうございます...!
指摘を受けてなるべく早く修正コミットをだします！

ikngtty · 2020-07-14T15:35:52Z

プルリクコメントに仕様書いてるけど、プルリクコメントだと流れちゃうよ。
多分自分でも忘れたりして見返すだろうし、ちゃんとした場所に残しておくのがオススメ。
READMEとか、helpコマンドとか。

daima3629 · 2020-07-14T15:38:35Z

プルリクコメントに仕様書いてるけど、プルリクコメントだと流れちゃうよ。
多分自分でも忘れたりして見返すだろうし、ちゃんとした場所に残しておくのがオススメ。
READMEとか、helpコマンドとか。

READMEはこのPRが無事Mergeされたあとにすぐ書こうと計画してます。
helpコマンド、というかUI全般もその後一新して見やすくしようと思ってます
ご指摘ありがとうございます。

ikngtty · 2020-07-14T15:46:16Z

第2引数(可変)に送りたいTODO文字列を指定

第2引数が空でも動くように見える。空の TODO を送りつけられる仕様はどうなんだろう。
普通は「TODO パラメータ入れ忘れてますよ」っつってキャンセルすると思う。
（作者がそれでいいならまあいいか、程度）

daima3629 · 2020-07-14T15:53:22Z

第2引数(可変)に送りたいTODO文字列を指定

第2引数が空でも動くように見える。空の TODO を送りつけられる仕様はどうなんだろう。
普通は「TODO パラメータ入れ忘れてますよ」っつってキャンセルすると思う。
（作者がそれでいいならまあいいか、程度）

確かにこの設計はまずいですね...修正します、ありがとうございます！

ikngtty · 2020-07-14T15:57:21Z

cogs/todo.py

+        dm_msg = f"""
+>>> {ctx.author.mention}さんからTODOリクエストが届きました。
+
+内容:
+・{todo}
+
+リクエストID: `{req_id}`
+
+このリクエストを承認する場合は`todo!request_approve {req_id}`
+拒否する場合は`todo!request_deny {req_id}`
+とコマンドを実行してください。"""


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

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

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

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

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

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

ikngtty · 2020-07-14T16:03:28Z

cogs/todo.py



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

+    def make_request(self, ctx, to_user, todo):


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

ikngtty · 2020-07-14T16:09:36Z

cogs/todo.py



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

+    def make_request(self, ctx, to_user, todo):


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

ikngtty · 2020-07-14T16:32:00Z

cogs/todo.py

+        data = {
+            "author": ctx.author.id,
+            "to": to_user.id,
+            "content": todo
+        }
+        self.bot.data["request"][hashed[:10]] = data


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

e.g.

クラスと違って、データの仕様（どんなプロパティがあるか）がどこかにまとまってるわけではない。

スペルミスしてもエラーが出てくれない。

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

ikngtty · 2020-07-14T16:42:32Z

cogs/todo.py

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


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

リクエストのデータを作成

作成したデータを bot オブジェクト内に格納

作成したデータを外部ファイルに保存（bot.save_data の呼び出し）

作成したリクエストの ID を返す

の４つだね。

問題点：

一つのメソッドが複数のことをやっている。単一責任原則に反している。

一度に複数のことを実現しようとすると、人は混乱するし、バグを生みがち。

その他詳しくは「単一責任原則」でググれ。

名前が実体を表していない（内容 1 は合っているが、内容 2 〜 4 は名前に表れていない）。

「メソッド名を見ただけで何をしているかが分かる」という状況が理想。そういう状況だと、脳に負担をかけずに読むことができる。今の状況だと、make_request を使っている部分を読む時に、「ええと、make_request は本当はリクエストの保存をしているから…」という感じで、make_request の中身まで頭に叩き込まないと読めないようになってしまっている。こういう状況では全体を俯瞰してチェックしたりするのが困難になる。

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

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

ikngtty · 2020-07-14T16:53:25Z

cogs/todo.py



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

+    def make_request(self, ctx, to_user, todo):


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

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

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

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

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

ikngtty · 2020-07-14T17:07:49Z

cogs/todo.py

+        await msg.add_reaction("👍")
+        await msg.add_reaction("👎")
+
+        def check(reac, user):


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

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

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

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

ikngtty · 2020-07-14T17:13:15Z

cogs/todo.py

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


なんで全体的に引用メッセージ（>>>がついてるやつ）なの？

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

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

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

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

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

ikngtty

思ったよりクリティカルな問題もなかったし、不格好なところもなかった。~~つまんねーの。~~
なのでとりあえず Approve で、全部ただのコメントにしておく。好きにマージして。

良い設計（クラス分けとか）に関する感覚みたいなのがまだ無さそうなので、そのへん勉強すると捗ると思う。

ArakiTakaki · 2020-07-15T15:30:01Z

cogs/todo.py

@@ -47,13 +60,14 @@ async def on_message(self, message):
        msg = await message.channel.send(msg)
        await msg.add_reaction("👍")


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

ICON_GOOD = "👍"

的な・・・？

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

ikngtty · 2020-07-22T12:49:59Z

c2b8478 「変数にまとめた」ってコミットメッセージだけど、変数と定数は違うぞ（細かい定義は言語によるところあるけど）

daima3629 · 2020-07-22T13:00:23Z

c2b8478 「変数にまとめた」ってコミットメッセージだけど、変数と定数は違うぞ（細かい定義は言語によるところあるけど）

あっ...素でミスりました()
コミットやり直ししときます()

daima3629 · 2020-07-23T05:24:01Z

皆さんのご指摘のおかげでかなりいいコードにすることができたと思います。
いくつかを除き、全てのレビューを反映させました。
今回は反映させなかったいくつかのレビューもいつかは反映させたいと思っています。
そろそろMergeをしようかと思っていますが、もし更になにかあればぜひお願いします。

ikngtty · 2020-07-23T06:40:18Z

cogs/todo.py

+import numpy
+import textwrap
+
+REACTIONS = ["👍", "👎"]


REACTIONS[0] が up で REACTIONS[1] が down なのは非自明。どっちがどっちかすぐ間違えちゃいそう。

REACTION_AGREE と REACTION_DISAGREE だとか、
REACTION_YES と REACTION_NO みたいな感じで、
役割を明らかにした名前にするのが良い。

あ、リアクションの集合を表す REACTIONS も必要みたいね。
そしたら

REACTIONS = [REACTION_YES, REACTION_NO]

みたいにするのがいい。

あーなるほど！
そうさせていただきます！

ikngtty · 2020-07-23T06:44:35Z

cogs/todo.py

+        await msg.add_reaction(REACTIONS[0])
+        await msg.add_reaction(REACTIONS[1])


REACTIONS の中身全部 for 文とかで add するようにした方が間違い無さそう。
（例えばリアクションの種類を増やしたくなった時とかに、こういう箇所での追加漏れを防ぎやすい。）

chun37 · 2020-07-23T07:17:29Z

cogs/todo.py

@@ -45,22 +61,23 @@ async def on_message(self, message):
            msg += f"{todo}\n"
        msg += "\nTODOに追加しますか？"
        msg = await message.channel.send(msg)


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

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

chun37 · 2020-07-23T07:27:12Z

cogs/todo.py

            return await ctx.send(f">>> {ctx.author.mention}のTODOリスト\n\nNone")

-        for i, todo in enumerate(l):
+        for i, todo in enumerate(todo_list):
            todos += f"{i}: {todo}" + "\n"


todos の追加の仕方がスマートじゃない気がする
enumerate を使ってるのは良い
空文字用意 → 必要なものと改行を結合 → 最後の改行を消す
って処理するくらいなら、リスト内包表記使って join するのが良いと思う

chun37 · 2020-07-23T07:32:26Z

cogs/todo.py

+
+        reaction, _ = await self.bot.wait_for("reaction_add", check=check)
+        if str(reaction.emoji) == REACTIONS[0]:
+            if not self.bot.data["todo"].get(str(message.author.id)):


Suggested change

if not self.bot.data["todo"].get(str(message.author.id)):

if str(message.author.id) not in self.bot.data["todo"]:

get を使ってると、ぱっと見た時何書いてるかわからなかった

has_key の代用で key in dict が出来るので、そちらのほうが良いと思われる

chun37 · 2020-07-23T07:38:02Z

cogs/todo.py

+            deleted_todo = self.bot.data["todo"][str(ctx.author.id)][num]
+            del self.bot.data["todo"][str(ctx.author.id)][num]


Suggested change

deleted_todo = self.bot.data["todo"][str(ctx.author.id)][num]

del self.bot.data["todo"][str(ctx.author.id)][num]

deleted_todo = todo_list[num]

del todo_list[num]

上で todo_list 定義してるんだし、そのまま使えば良さそう

todo_list内のデータを削除すればself.bot.data内のデータも一緒に削除できるんですか？

あ、検証してみたところ消されました
Pythonの参照渡しというのはdict.getでも働くんですね...
ありがとうございます！

Python は全部「値渡し」だよ。「Python の引数は参照渡し」ってデマ書いてるサイト、めちゃくちゃ多いけど。

cf.
https://qiita.com/ur_kinsk/items/949dabe975bdc1affb82
https://qiita.com/raccy/items/d4c5e7995a8fc90109ee

このへん一度ちゃんと理解したら、検証しなくても色々一発で予想できるようになると思う。

chun37 · 2020-07-23T07:47:37Z

cogs/todo.py

+    @request.command(aliases=["a"])
+    async def approve(self, ctx, req_id: str):
+        req_todo = self.bot.data["request"].get(req_id)
+        if not req_id:


Suggested change

if not req_id:

if not req_todo:

ミス？

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

sizumita · 2020-07-28T02:54:55Z

cogs/todo.py

+        dm_msg = f"""\
+        >>> {ctx.author.mention}さんからTODOリクエストが届きました。
+
+        内容:
+        ・{todo}
+
+        リクエストID: `{req_id}`
+
+        このリクエストを承認する場合は`todo!request approve {req_id}`
+        拒否する場合は`todo!request deny {req_id}`
+        とコマンドを実行してください。
+        """
+        try:
+            await member.send(textwrap.dedent(dm_msg))
+        except discord.Forbidden:
+            await ctx.send(f"{member.mention}\n" + textwrap.dedent(dm_msg))


ここIDだけではなく(TODOを追加するところと同じように)リアクションでできたら良さそうかな〜と思いました

そこどうしようか迷ってたんですが、やっぱリアクション確認入れたほうがいいですね
ありがとうございます！

daima3629 · 2020-08-02T07:56:00Z

少し設計を変更させていただきました。

Commandrequest, Commandrequest_approve, Commandrequest_denyをcommand.groupで統合
requestはrequest createへ変更
request_approveとrequest_denyはrequest respondへ統合, リアクションで承認するか拒否するか指定するように
(おまけ)それぞれエイリアスを設定

daima3629 added 2 commits July 3, 2020 22:25

temp commit

645304e

[Add] TODOリクエスト機能を追加

feb0807

chun37 reviewed Jul 13, 2020

View reviewed changes

[Fix] リクエストIDを36進数に圧縮したメッセージIDに変更

624a42c

ikngtty reviewed Jul 14, 2020

View reviewed changes

[Fix] エラーキャッチ範囲の限定可&不要f-string消去

f34922b

ikngtty reviewed Jul 14, 2020

View reviewed changes

ikngtty approved these changes Jul 14, 2020

View reviewed changes

ArakiTakaki reviewed Jul 15, 2020

View reviewed changes

[Fix] 文章修正, みやすさUP, 内部関数の明確化など

e6c17db

[Fix] リアクションの絵文字を定数にまとめた

4b300f0

daima3629 force-pushed the feature/request branch from c2b8478 to 4b300f0 Compare July 22, 2020 13:11

daima3629 added 2 commits July 22, 2020 22:30

[Fix] 変数名修正, 無意味なキャスト消去, linterに沿い改行の数調整

eaf6c57

[Fix] 細かい修正2つ

4d73f4e

[Fix] split("\n")からsplitlinesに変更

41a3dd4

ikngtty reviewed Jul 23, 2020

View reviewed changes

chun37 reviewed Jul 23, 2020

View reviewed changes

daima3629 added 3 commits July 24, 2020 18:49

[Fix] リアクション用絵文字の明確化

6b3fa3a

[Fix] ミス修正

0ebd49b

[Fix] 変数名の使いまわしを修正

ae5c019

chun37 approved these changes Jul 27, 2020

View reviewed changes

sizumita reviewed Jul 28, 2020

View reviewed changes

daima3629 added 2 commits August 2, 2020 16:45

[Fix&Change] リアクション関係の修正, 設計変更

8472512

[Fix] 文章の修正

c2d2ddf

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

		@@ -47,13 +60,14 @@ async def on_message(self, message):
		msg = await message.channel.send(msg)
		await msg.add_reaction("👍")

		await msg.add_reaction(REACTIONS[0])
		await msg.add_reaction(REACTIONS[1])

	if not self.bot.data["todo"].get(str(message.author.id)):
	if str(message.author.id) not in self.bot.data["todo"]:

		deleted_todo = self.bot.data["todo"][str(ctx.author.id)][num]
		del self.bot.data["todo"][str(ctx.author.id)][num]

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

Are you sure you want to change the base?

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

Conversation

daima3629 commented Jul 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daima3629 Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chun37 Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

chun37 commented Jul 13, 2020

daima3629 commented Jul 13, 2020

ikngtty commented Jul 14, 2020

daima3629 commented Jul 14, 2020

ikngtty commented Jul 14, 2020

daima3629 commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikngtty Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikngtty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikngtty commented Jul 22, 2020

daima3629 commented Jul 22, 2020

daima3629 commented Jul 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daima3629 commented Aug 2, 2020

daima3629 Jul 14, 2020 •

edited

Loading

chun37 Jul 13, 2020 •

edited

Loading

ikngtty Jul 14, 2020 •

edited

Loading