helpdm for participants#1629
Conversation
Xithrius
added
a: backend
ChrisLovering
left a comment
ChrisLovering
left a comment
There was a problem hiding this comment.
Just a few comments,. overall looks pretty good :D
Co-authored-by: ToxicKidz <78174417+ToxicKidz@users.noreply.github.com>
|
All requested changes have been done. |
ChrisLovering
left a comment
ChrisLovering
left a comment
There was a problem hiding this comment.
Just one last change and the rest looks good. I've tested locally too and all seems well.
After the requested change is done, I'll merge in main and do a final test
|
@ToxicKidz thinking about it, I am not too sure about the 0, 1 to toggle the command. |
If you was to add |
|
I was thinking of adding the bool converter which is pretty useful, it's still case insensitive and supports |
|
What about just having |
That is a really good suggestion, 👍 |
| @staticmethod | ||
| def _serialise_session_participants(participants: set[int]) -> str: | ||
| """Convert a set to a comma separated string.""" | ||
| return ','.join(str(p) for p in participants) | ||
|
|
||
| @staticmethod | ||
| def _deserialise_session_participants(s: str) -> set[int]: | ||
| """Convert a comma separated string into a set.""" | ||
| return set(int(user_id) for user_id in s.split(",") if user_id != "") |
There was a problem hiding this comment.
Aren't sets in redis more appropriate for this sort of thing? It would remove the need to serialise and deserialise (unless you count str -> int as deserialisation). It wouldn't even need to retrieve all items since there are commands to check set membership.
There was a problem hiding this comment.
Async Rediscache doesn't support sets. We were thinking about using sets but when we figured out it doesn't support it, we switched to that.
There was a problem hiding this comment.
aioredis does support it and async-redis cache can trivially be extended to support it (yes I think it's worth doing that).
There was a problem hiding this comment.
Note I'm not suggesting implementing the set operations with all the type conversion stuff. Just a basic SADD call or whatever that will store a string as given.
There was a problem hiding this comment.
I'm not too familiar with Redis can you please give more information about how I would implement this? Also I think it would be best to do this as a separate feature.
There was a problem hiding this comment.
That's alright. Let's just leave your implementation for now. I can try to merge set support into async-rediscache in the future.
There was a problem hiding this comment.
Alright, sounds good!
|
Those weren't there before.... But as soon as I push they show up. No changes were made that would break anything. |
Co-authored-by: ToxicKidz <78174417+ToxicKidz@users.noreply.github.com>
6 participants
closes #1567
This PR is for the help channel participant DMs, me and Griff went on and tested this in and we ran into no problems at all, we even went as far as testing it with multiple people.
This is the embed that the user receives in DMs.
Also added feedback if the user already has help DMs turned on already(this is the same for when the user turns them off)
