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

SlashCommand: tell the user when a parameter is missing #835

Closed
wants to merge 1 commit into from
Closed

SlashCommand: tell the user when a parameter is missing #835

wants to merge 1 commit into from

Conversation

dijkstracula
Copy link

Hello!

This is my attempt at addressing the main issue that I filed #829 . This patch does not touch any of the subsequent comments in that issue (I do not warn if an extraneous argument is passed to a single-parameter command or attempt to deal with malformed slash commands like simply typing /; nor do I touch anything on the backend). matchingSlashCommand now returns, morally, a union: either a successful SlashCommand object or one of two failure strings, which the caller matches on. This allowed the caller's logic to be simplified a bit, which is nice.

I am not a typescript person (or, really, a frontend person at all) so I hope this is an idiomatic solution. here is a REPL with some tests.

I was hoping that I'd be able to attach a local frontend to the running backend during this week's Celebration, but it looks like from the README that I still require an API key and a FireBase thingie to do so? If there's a trivial way of pointing it to prod then I'm happy to test it that way, of course.

@dijkstracula dijkstracula closed this by deleting the head repository Aug 3, 2024
@lazerwalker
Copy link
Collaborator

Hey @dijkstracula! Sorry for the delayed response, we operate a bit on "understaffed volunteer" time 😅

This looks broadly reasonable, and I'd love to help you get this merged in (pointing at prod does require private Firebase credentials for now, but we have a new branch getting merged after this year's event that will fix that), but it's not currently mergeable since you deleted your fork. No idea if it's easy for you to restore or if you're still interested contributing, but I'd love to get this in if you still are.

Thanks!

@dijkstracula
Copy link
Author

Hi @lazerwalker,

No worries - luckily I still had my local fork on my machine, so I re-forked and pushed again. It has the same branch name so I think everything should just hook up again as it was before!

Thanks!

@dijkstracula
Copy link
Author

Spoke too soon - I'll create a new PR for this!

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