search

serenity-rs / poise

Discord bot command framework for serenity, with advanced features like edit tracking and flexible argument parsing
MIT License
653 stars 114 forks source link

Rework permission checks #286

Open jamesbt365 opened 3 months ago

jamesbt365 commented 3 months ago

This shouldn't be breaking in the traditional sense, but does theoretically change behaviour in one case.

Previously, when the bots permissions could not be checked, it let the command run, now it fires a UserPermissionsError, there isn't really a nice way of checking where the error came from because the only place this can happen is in http requests.

The guild, channel or member itself could be the failure point, the only place we can now tell is directly on which member failed to fetch and return the "right" error from there.

I've placed a comment in the code about it, I just want to check if this is okay or if I should attempt to get the right error even though the only place it can come from is http.

I can pull the member from the cache too if that is desirable, but developers without GUILD_MEMBERS may suffer from this change. I believe checking for this is doable by directly accessing Http, but this is currently out of scope and can be handled later if that is desirable.

In serenity-next the channel http request has already been removed and I'd rather not mess around doing that bit manually and falling back to http if it isn't cached.

CURRENTLY UNTESTED.

GnomedDev commented 1 week ago

Okay, currently this code is a mess because of cache and no cache support, and I see you adding the signature Option<(Option<_>, Option<_>)> which is sus to start with.

Can you abstract the distinct operations that need difference between cache and http into functions in a different file that we can switch between?

jamesbt365 commented 1 week ago

Lets go from where the most recent commit is, what else ideally should be done to make this more presentable and maintainable?