Skip to content

Helpermessage and respective changes to helper #38

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

Open
wants to merge 24 commits into
base: rewrite
Choose a base branch
from

Conversation

Indiance
Copy link
Contributor

Followed @ilikecubesnstuff 's guidelines on the creation of helpermessage which includes:

  • create - creating a helpermessage given the channel and role, sends a helpermessage to the given channel
  • delete - delete a helpermessage given the channel
  • edit - edits the content of all helpermessages by editing the descriptions
  • list - list all the active helpermessages
  • auto-updating: checks if there has been a change in roles specifically, checks if the added role is a subject helper role (i.e., has a valid helpermessage connected to it), and appeds the name of the list. If a person is being dehelpered, the same check is conducted except for the deleted role, and the mention is removed.

Copy link
Collaborator

@ilikecubesnstuff ilikecubesnstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much, Indiance! I can see a few changes to make here:

  • Although channel-role relations are to be read from config.toml, we should store the helper message IDs using db. See other cogs for examples on how to use this - namely, we should be editing the db on helpermessage creation and deletion specifically.
  • Currently the description is hard-coded, but ideally this is also to be read from config.toml.
  • The embed_getter method appears buggy - I have suggested some changes, particularly writing it as a generator so you can account for all the relevant helpermessages in a loop.

@NathanealV NathanealV linked an issue Jul 3, 2024 that may be closed by this pull request
Copy link
Collaborator

@NathanealV NathanealV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR! Please see comments for specific changes/questions.

In terms of other comments:

  • General consistency - adding periods to the end of docstrings and messages sent to discord channels would be great.
  • Adding logger statements for helpermessage create, delete, and description editing would help with debugging.

Copy link
Collaborator

@ilikecubesnstuff ilikecubesnstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please run Ruff once on this branch—many strings include single quotes when they should all have double quotes.

I'm also not sure how changing the HelperMessage model will involve things like migrations—Nath probably knows better.

For now, this looks good. My nitpicks are only grammatical, such as amendments in docstrings and error messages.

.gitignore Outdated
@@ -130,4 +130,5 @@ dmypy.json

config.json
config.toml
bot/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this folder for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally shouldn't be there since its a folder to my venv. changing it

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.

Helper Message
3 participants