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

Fix /tell override crash when used without recipient or message #113

Merged
merged 5 commits into from
Jul 10, 2023

Conversation

S-S-X
Copy link
Member

@S-S-X S-S-X commented Jul 8, 2023

Some tests for #112

When done, fix #112

@S-S-X S-S-X marked this pull request as ready for review July 8, 2023 20:58
@S-S-X
Copy link
Member Author

S-S-X commented Jul 8, 2023

Prob fine but not sure how is_muted should actually behave if target is nil, despite what I said before I think I'm leaning towards letting it crash.

Maybe it should throw warning to logs, after all it is something that should not normally happen.

Or maybe after all it shouldn't actually check for invalid input but just let it crash so that actual underlying issue, that might well break other things, wont get hidden.

@S-S-X
Copy link
Member Author

S-S-X commented Jul 9, 2023

am going to change it and make it clear that target is needed for mute check.

@S-S-X
Copy link
Member Author

S-S-X commented Jul 9, 2023

Changed to allow original mesecons /tell to handle empty target parameter.
And changed pattern to allow calling through checks even if there was no message after recipient.
And changed is_muted resilience to assertion to make responsibilities of caller clear and to keep root cause visible.

@BuckarooBanzay what do you think about update? My reasoning for changes is that I think first version was actually very bad because it would hide root cause for issues that could anyway easily emerge elsewhere in callback chain.

@S-S-X S-S-X changed the title Test before_send_pm invalid input Fix /tell override crash when used without recipient or message Jul 9, 2023
@BuckarooBanzay
Copy link
Member

@BuckarooBanzay what do you think about update? My reasoning for changes is that I think first version was actually very bad because it would hide root cause for issues that could anyway easily emerge elsewhere in callback chain.

looks good 👍

@S-S-X S-S-X merged commit 2a7a1ea into master Jul 10, 2023
4 checks passed
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.

/tell not handling self recipient
2 participants