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

Allow prefix change #45

Closed
aquelemiguel opened this issue Oct 22, 2021 · 15 comments · Fixed by #69
Closed

Allow prefix change #45

aquelemiguel opened this issue Oct 22, 2021 · 15 comments · Fixed by #69
Assignees
Labels
💬 discussion Further information is requested ✨ feature New feature or request 🐤 good first issue Good for newcomers
Milestone

Comments

@aquelemiguel
Copy link
Owner

Forcing users to use the ! prefix can cause conflicts with other bots on the server.

Should this be an environment variable (easier but requires redeploying) or does it justify a whole new !prefix command usable only by admins (harder but allows for changing on the fly)? Requesting your inputs here, @afonsojramos and @joao-conde.

Regardless of the chosen approach:

  • The bot's activity should also reflect the prefix change.
  • Existing messages that reference the exclamation mark prefix should now use the current prefix.
@aquelemiguel aquelemiguel added ✨ feature New feature or request 🐤 good first issue Good for newcomers 💬 discussion Further information is requested 🎃 hacktoberfest Suitable for Hacktoberfest labels Oct 22, 2021
@aquelemiguel aquelemiguel added this to the Stability milestone Oct 22, 2021
@joao-conde
Copy link
Collaborator

Env variable would mean redeploy, so it would be more like, each server owner picks one and launches the bot and that is it.

The !prefix command seems good, but can you change the prefix on the fly? Does serenity allow it? With that I mean, could a command access this framework (https://github.com/aquelemiguel/parrot/blob/main/src/main.rs#L72) and configure it with a new prefix? If it can, I suggest actually both approaches: adding an env var for flexibility that defaults to !, but also a command.

@afonsojramos
Copy link
Collaborator

afonsojramos commented Oct 22, 2021

I think that the environment variable is out of the question because one deployment of the bot can work on multiple servers (just tested it to verify).

What this means is that multiple servers can have different prefixes. As such, I think we should opt for something dynamic so that they can change prefixes at will.

Additionally, we should save the prefix in some file so that it gets associated with the server ID in case of a reboot. This way the prefix is always saved and restored.

@afonsojramos
Copy link
Collaborator

afonsojramos commented Oct 22, 2021

I'll try to take on this issue if everyone is okay with it ✌

@joao-conde
Copy link
Collaborator

joao-conde commented Oct 22, 2021

Give it to @Dannyps , if he wants it, because he is short on PRs for Hacktoberfest

@Dannyps
Copy link
Contributor

Dannyps commented Oct 22, 2021

Merci. Going with the customisable prefix stored somehow. Files may be tricky because of permissions, though. Any input on this?

@afonsojramos
Copy link
Collaborator

I already started implementing 🥴

@Dannyps
Copy link
Contributor

Dannyps commented Oct 22, 2021

I'll kill you 🔫

@afonsojramos
Copy link
Collaborator

@Dannyps #46 seems easy!

@Dannyps
Copy link
Contributor

Dannyps commented Oct 22, 2021

I'll admit I'm having difficulty considering this is my first interaction with Rust. I'm keeping an eye on you. Stashed!

@aquelemiguel
Copy link
Owner Author

I'm locking this one to @afonsojramos then.

@aquelemiguel
Copy link
Owner Author

@afonsojramos Any advancements on this? 🙂

@aquelemiguel aquelemiguel removed the 🎃 hacktoberfest Suitable for Hacktoberfest label Dec 29, 2021
@joao-conde
Copy link
Collaborator

I think this issue and the listening party mode both require storing simple data but that needs to be associated with the server that called the command. As we discussed, a single parrot instance can then be used by multiple servers. The way current commands work for parrot running in the current server, is by the received context (which allows us to extract the server and a lock to the parrot instance on that discord server).

For this prefix issue and for others, we want to store simple settings but only for the discord server that called it.
I think we need to discuss several approaches for this and then pick one, before moving on. The current one I think we mostly agree on would be to store something like a JSON file where for each server ID you store settings (like, prefix). But we should investigate more.

@afonsojramos
Copy link
Collaborator

afonsojramos commented Jan 4, 2022

I'm already mid-development on this, I just need some time to figure out the inner workings of serenity.

In theory, it should be easy by using dynamic prefixes.

@aquelemiguel
Copy link
Owner Author

In theory, it should be easy by using dynamic prefixes.

Good find, looks promising!
Joining your suggestions, I'd ask if this approach looks correct:

  • Let a prefixes file map guild IDs to prefixes.
  • When booting the bot, if this file already exists, import it into context.data.
  • If this specific guild could not be found in the context, use the default Serenity prefix ~. Send a message prompting server owners to use the command ~prefix to change it.
  • Via dynamic_prefix, set a prefix based on the guild from which the message came from.
  • Implement a ~prefix command that both updates the context and the prefixes file with the new mapping.

@joao-conde
Copy link
Collaborator

Great, this is what I wanted: https://docs.rs/serenity/latest/serenity/client/struct.Client.html#structfield.data
Essentially we can store data and it is passed to each context, from what I understood. That solves the issues with !autopause command and !prefix. Except it does not guarantee persistency, i.e., when the bot is restarted. I would almost suggest we do it in separate issues and deal with persistence later. If not, we can solve it now by a simple JSON file, I think, because I dislike having a whole database layer added just for one setting right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 discussion Further information is requested ✨ feature New feature or request 🐤 good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants