Skip to content

Conversation

tmcdonnell2
Copy link
Contributor

@tmcdonnell2 tmcdonnell2 commented Mar 18, 2023

…r ";" from potential incorrect commands. Also, updates to SlashCommandEducatorTest to include new tests.

Closes #804 .

…r ";" from potential incorrect commands. Also, updates to SlashCommandEducatorTest to include new tests.
@tmcdonnell2 tmcdonnell2 requested review from a team as code owners March 18, 2023 21:06
@CLAassistant
Copy link

CLAassistant commented Mar 18, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@Zabuzard Zabuzard left a comment

Choose a reason for hiding this comment

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

Good stuff. Seems that you have to run spotless for auto-formatting though :)

…rect formatting. RawReminderTestHelper I think was caught up in the whole project autoformat as well.
@tmcdonnell2
Copy link
Contributor Author

I got the merge going again. I got Spotless working and should now pass. I am unsure about the other workflow programs.

Spotless suggested changing a method in SlashCommandEducator because the parameter being passed in was already a final in the class and thus didn't need to be passed in and could be accessed freely.

One other thing I couldn't figure out (and wasn't sure if it made sense) was to try and make the response ephemeral? I think the classes involved can't do this and I don't know if its what you want.

…rect formatting. RawReminderTestHelper I think was caught up in the whole project autoformat as well.
Zabuzard
Zabuzard previously approved these changes Mar 20, 2023
@Zabuzard
Copy link
Member

Thanks, I like it 👍

One other thing I couldn't figure out (and wasn't sure if it made sense) was to try and make the response ephemeral? I think the classes involved can't do this and I don't know if its what you want.

Yeah, thats not possible. Ephemeral only works if you have a client interaction to interact with. For example if the user invoked a slash command, you can respond ephemeral to that. But you can not send ephemeral messages "out of nowhere".

@Zabuzard
Copy link
Member

Zabuzard commented Mar 20, 2023

Seems that the unit tests fail for one of the strings:

SlashCommandEducatorTest > sendsAdviceOnMessageCommand(String) > SlashCommandEducatorTest.sendsAdviceOnMessageCommand(String)[7] FAILED

WantedButNotInvoked at SlashCommandEducatorTest.java:49

Seems to be that one: "this is a test;"

Which makes sense, since the regex classifies any message ending with semicolon a non-command. But I think thats also what we want. So maybe move that string to the provideOtherMessages test instead.

@tmcdonnell2 tmcdonnell2 requested a review from Taz03 March 20, 2023 17:43
@tmcdonnell2 tmcdonnell2 requested a review from Zabuzard March 20, 2023 17:57
Copy link
Member

@Zabuzard Zabuzard left a comment

Choose a reason for hiding this comment

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

awesome! 🎉

@Zabuzard Zabuzard merged commit 09d4ce3 into Together-Java:develop Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SlashCommandEducator should ignore method calls
4 participants