Potential fix for #26#30
Conversation
…`chad` to `patrick` (rip King).
paulikauro
left a comment
paulikauro
left a comment
There was a problem hiding this comment.
Cool! Some notes in the comments to address
| val channelId: Long = 1234L, | ||
| val chadId: Long = 1234L, | ||
| val channelId: Long = 1234L, // game-chat | ||
| val patrickId: Long = 1234L, |
There was a problem hiding this comment.
Changes in config like this require a matching migration in Config.kt, so that we don't have to manually edit config files unless actually necessary.
For the comment: Better to remove it and instead rename the channelId field itself to reflect the usage, if it's important. I think channelId is ok, but gameChatChannelId would indeed be more explicit. (And this would also require a migration)
There was a problem hiding this comment.
Another thought here would be to have an allowedBots list of IDs instead. It'd be a bit cleaner, if not a bit overengineered for our needs. (Although I can see that being useful on a testing server with multiple game chat bridges and Patrick instances... hmm)
There was a problem hiding this comment.
Yeah, I thought about a list too, but I didn't want to put too much into this PR. If you say these should come together though, I'll work on that today or tomorrow.
There was a problem hiding this comment.
It's fine to put it in this PR since it's being changed anyway (saves one config migration later too 😎). I think it's a nice idea so ye go ahead
|
|
||
| // Bot is patrick. We add a '[Bot]' prefix. | ||
| // See https://github.com/OpenRedstoneEngineers/ChattORE/issues/26. | ||
| displayName = "[Bot] " + displayName |
There was a problem hiding this comment.
While this is the easiest way to attach the bot prefix, it doesn't stand out from the rest of the display name. (Someone could nick themselves [Bot] SomeBot in Discord and you wouldn't be able to tell the difference. In practice, not a problem, but it highlights the issue.) We should think if we want a prefix that stands out more (or how much we even need the prefix in the first place). That can also be addressed later, this already solves the issue
| val channelId: Long = 1234L, | ||
| val chadId: Long = 1234L, | ||
| val channelId: Long = 1234L, // game-chat | ||
| val patrickId: Long = 1234L, |
There was a problem hiding this comment.
It's fine to put it in this PR since it's being changed anyway (saves one config migration later too 😎). I think it's a nice idea so ye go ahead
2 participants
Also renamed mentions of
chadtopatrick(rip King), and renamed a var once, where it makes the function a bit clearer.See #26.