mcb-dev / mCodingBot

The Discord bot for the mCoding Discord server.
https://mcoding.io/discord
MIT License
12 stars 4 forks source link

Highlight Cooldowns #86

Closed CircuitSacul closed 1 year ago

CircuitSacul commented 1 year ago

This makes a few changes to improve highlights

These are just my feel for how it should work. If you think it should be tweaked lmk.

MithicSpirit commented 1 year ago

These changes sound good to me, though please make the two cooldowns relatively easy to configure (like MAX_HIGHLIGHTS and MAX_HIGHLIGHT_LENGTH).

Might be interesting for us to look into having a proper config file for these things in the future (maybe we can commandeer the config.json for these purposes?).

CircuitSacul commented 1 year ago

These changes sound good to me, though please make the two cooldowns relatively easy to configure (like MAX_HIGHLIGHTS and MAX_HIGHLIGHT_LENGTH).

Might be interesting for us to look into having a proper config file for these things in the future (maybe we can commandeer the config.json for these purposes?).

Well these things would undoubtedly be defined in config.json (with defaults in config.py). Unless you meant per-user configuration? I think that's possible too.

Lunarmagpie commented 1 year ago

Well these things would undoubtedly be defined in config.json (with defaults in config.py). Unless you meant per-user configuration? I think that's possible too.

I think moving all config options to config.py would be a good idea. May open a PR for that before this one gets merged.

MithicSpirit commented 1 year ago

Well these things would undoubtedly be defined in config.json (with defaults in config.py). Unless you meant per-user configuration? I think that's possible too.

MAX_HIGHLIGHTS and MAX_HIGHLIGHT_LENGTH aren't in config.json :p. As for per-user configuration, it would be nice but probably overkill for now.

CircuitSacul commented 1 year ago

MAX_HIGHLIGHTS and MAX_HIGHLIGHT_LENGTH aren't in config.json :p. As for per-user configuration, it would be nice but probably overkill for now.

Oh oof. They should've been. Well, I'll be putting these settings in config. And yeah, per-user config is outside the scope of this pr anyways

CircuitSacul commented 1 year ago

This is done. Ready for review.

Lunarmagpie commented 1 year ago

Still would prefer if NamedTuple was used.

CircuitSacul commented 1 year ago

Actually the bucket should probably be SentMessageBucket

I just named it "UserChannelBucket" b/c that's what it is literally

Lunarmagpie commented 1 year ago

I just named it "UserChannelBucket" b/c that's what it is literally

No reason to say the properties of the class. Makes more sense to name it so its clear where its being used.