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

Merging wonderful changes from @justinrusso, with some updates #14

Merged
merged 17 commits into from
Jun 21, 2021

Conversation

tomaarsen
Copy link
Owner

@tomaarsen tomaarsen commented Jun 21, 2021

Pull request overview:

  • Improved README - removed troublesome HTML tags, improved readability, and added new fields in settings.

  • Added BotOwner field - gives developer (or whichever user is specified here) the permission to bypass the cooldown, or use the generate command even when the command is disabled.

    • Convert this into a list of "Allowed Users", that can bypass cooldowns and always use the generate command.
  • Added ShouldWhisper field - whether the Bot should whisper the cooldown message. Otherwise, do not send the cooldown.

    • Rename to WhisperCooldown. ShouldWhisper might confuse users, who believe that it means the generated output is whispered.
    • Still always whisper in certain situations, such as when the broadcaster updates the cooldown by whispering the Bot.
  • Modified default outputs slightly (e.g. after using !enable).

  • Added EnableGenerateCommand field - enables or disables !generate. Broadcaster can override this setting, and still generate.

    • Allow users in the list of "Allowed Users" described above to also override the disabling of !generate. Note: When the Bot is disabled through !disable, then nobody can use it. This still allows the broadcaster to prevent people from using it. (Edit: I see now that this is already done by you, as I see now that you updated self.check_if_streamer. I'll rename this method to something clearer instead.)
  • Added .gitignore.

    • It is indeed preferred to have the settings.txt (now settings.json) in the .gitignore. However, I do want to ensure that a correct sample settings.json is pushed to GitHub, allowing users to see what the file should look like, before running the program.
  • Renamed settings.txt with settings.json.

    • Update the Settings class to include an automatic update, which reads the old settings.txt, adds the default values of the new fields, writes it to settings.json, and removes settings.txt.
  • Added typing to parameters in methods.

    • Write better docstrings for the methods.
  • Start a second sentence if the generated sentence is too short, uses a new MinSentenceWordAmount value in settings.json.

    • Ensure that this works correctly, and investigate whether the inclusion of a dot or comma is preferred here.
  • Set the default help Timer to once every 5 hours, instead of off.

  • Ensure the README is correct, with the new changes.

@justinrusso

First of all, thank you! I really like the direction they went, and I'd love to take some of the changes one step further, as described by the boxes.
I'll work on these myself, though your help is definitely appreciated. For the time being, I'd really love to hear your opinion on the changes I suggested to your updates.

Upon completion of these changes, and when the new updater_1 is feature-complete and ready, I'll happily merge this to master, and mention you as a contributor on the README page, if you'd like!

  • Tom Aarsen

@tomaarsen tomaarsen merged commit 6344a12 into tomaarsen:updater_1 Jun 21, 2021
@justinrusso
Copy link
Contributor

Happy you liked it!

I'll happily merge this to master, and mention you as a contributor on the README page, if you'd like!

This is not necessary but thank you for the offer

Ensure that this works correctly, and investigate whether the inclusion of a dot or comma is preferred here.

This has seemed OK so far in practice. I do use an auto-generate timer and after a few days I do end up getting an error thrown but I believe it is unrelated. Have not had time to look into it.

It might be good to make it a setting. Maybe something like SentenceDelimiter with an empty string as the default?

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.

2 participants