Skip to content
This repository has been archived by the owner on Jul 24, 2023. It is now read-only.

Music command recognition #20

Merged
merged 6 commits into from Mar 16, 2021
Merged

Music command recognition #20

merged 6 commits into from Mar 16, 2021

Conversation

jonatep
Copy link
Contributor

@jonatep jonatep commented Mar 12, 2021

This code needs this pull request LuisMayo/objection_engine#27 in order to work fully, as a new argument has been added in the function "comments_to_scene".
When mentioning the bot, now you can add a music= argument, followed by 3 posibilities:
PWR: Just like the default bot, using music from the first game
JFA: Music from Justice for All
TAT: Music from Trials and Tribulations
If there is no music argument in the mention, it will be selected randomly between these options. However, if there is some typo in the arguments, the bot will remind the user how to write it properly
I'm sure this isn't perfect, so please tell me any mistakes I may have made :)

PD: The other person commiting this, "Julian", is also me, I forgot to configure the git user name and I don't know how to change it :P

@jonatep
Copy link
Contributor Author

jonatep commented Mar 12, 2021

I don't know if I should be the one resolving these conflicts that have appeared when commiting this pr, or if the author is the only one to do so

@LuisMayo
Copy link
Owner

LuisMayo commented Mar 12, 2021

We both can!

I can do it in the merge commit and you can do a rebase on top of current main commit

Doing a rebase is a bit more of a hassle than the merge commit so if you're kind of new to git just leave the conflicts to me :)

Thanks for your work!

@jonatep
Copy link
Contributor Author

jonatep commented Mar 14, 2021

Okay, it's good to know! Yep, I'm kind of new in GitHub so if it isn't much of an issue for you I would appreciate if you could handle the conflicts :)

Copy link
Owner

@LuisMayo LuisMayo left a comment

Choose a reason for hiding this comment

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

Hi!
After a review of the changes I think I'd need the following changes if you may accept to make them

  • Make "Phoenix Wright: Ace Attorney" the default if no soundtrack is selected. This is something I personally prefer but I won't manage this bot only on my preferences. However, a poll conducted with the bot users shows that the majority prefer it that way too

  • Add a Random option. I actually like the idea of letting the universe decide for us, so I think that maybe if the user specifies music=rnd, then the soundtrack could be chosen at random

  • Currently the way you're parsing the user input may lead to future errors if more user-provided options are to be introduced. You should either split by "music=" instead of by "=" (if that's possible I don't know) or use regular expressions. If you're having too much trouble with this one please not worry and I'll gladly do it :3

Again I cannot thank you enough your contribution ^^.

@jonatep
Copy link
Contributor Author

jonatep commented Mar 16, 2021

All done! It should still work without major issues. Thanks a lot for the feedback, glad to help! :)

@LuisMayo LuisMayo linked an issue Mar 16, 2021 that may be closed by this pull request
@LuisMayo LuisMayo merged commit d8c292c into LuisMayo:main Mar 16, 2021
@LuisMayo
Copy link
Owner

Done now!
Thanks for your work

@jonatep
Copy link
Contributor Author

jonatep commented Mar 17, 2021

Perfect! It was nothing, I hope everyone likes it :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add music from other Ace Attorney games
2 participants