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

Changed example commands regex #1006

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

seadog007
Copy link

@seadog007 seadog007 commented Sep 14, 2022

  • All tests pass (Didn't run it since I am not change the lib source)
  • I have run npm run doc

Description

Changed the examples and README.md since lots of people wrote codes with copying the example, which might cause the bot to match URL or other things.

Generally, the only thing that should consider as a command is a string start with special character (in this case "/")
Therefore, the following should be considering as a command

/echo 123
/help

but not these

I found a file under /bin, which is /bin/echo What is is file for in Linux? 
http://help.google.com/

Using the original example will resulting as
截圖 2022-09-15 上午1 19 42

References

Nope

@kamikazechaser
Copy link
Collaborator

You probably also want to add the regex terminator to some of these examples i.e. $

@seadog007
Copy link
Author

@kamikazechaser I did consider that, but since there might be something like /command@botname, and regex will be ^\/command(@.*|$).
I don't think complexing the example is a good idea then.
Therefore, I was not adding that to the PR, or should I?

@kamikazechaser
Copy link
Collaborator

/command@botname

This is usually in groups. Yeah I think you should add it for uniformity.

@danielperez9430
Copy link
Collaborator

danielperez9430 commented Sep 23, 2022

This regex is still wrong, becuase @.* is any character include espace and other text that not match.

/test@mytest_bot and /test@mytest_bot hello test is match.

This weekend I will review it

@seadog007
Copy link
Author

@danielperez9430
Well, I will simply ignore the argument for the commands that doesn't require args.
but for something like /echo, I did use ^\/echo(?:@.*?)? (.+), or you prefer ^\/echo(?:@.*?)?( .*|$) ?

@kamikazechaser
Copy link
Collaborator

Yeah I think you should add it for uniformity.

Btw I meant the regex terminator here, not adding @botname to the regex. Otherwise the PR is okay after that change.

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.

3 participants