search

go-chat-bot / bot

IRC, Slack, Telegram and RocketChat bot written in go
MIT License
824 stars 194 forks source link

Make using Unidecode optional #105

Closed oddlid closed 5 years ago

oddlid commented 5 years ago

This fixes #103

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 96.748% when pulling f59ade2c6e7a3f78681b3db54391933894d206df on oddlid:master into ece33b98f74209dd00d4915be15c11e14ca8aa67 on go-chat-bot:master.

fabioxgn commented 5 years ago

Hi @oddlid thanks for your contribution, but I'm thinking that it might have a better way for doing this than a global variable.

This behavior was introduced to allow typing commands with variants like "!cotacao" and "!Cotação", and I don't think that converting the args to unicode is a good idea in any case, as it can lead to misbehavior.

Would remove just the args parsing (leaving the command parsing) work for you? If so, we could get rid of this variable and just use unicode in the command.

oddlid commented 5 years ago

Hi @fabioxgn I agree a global variable isn't optimal. I just didn't want to introduce any change in default behaviour with my PR. Only parsing/translating the command, but not the args would work fine for me. What I'm doing is saving individual wecome messages/greetings for nicks. As we speak norwegian and swedish in my channels, translating the args messes up the messages. I only use ASCII for command names, so they wouldn't be affected.

Looking forward to the update, so I can base on upstream and not my own fork again :)

Thanks!

fabioxgn commented 5 years ago

@oddlid so just parse the command and remove the args parsing, it doesn't make sense to parse the args, this way we can avoid the configuration.

Thanks.

oddlid commented 5 years ago

@oddlid so just parse the command and remove the args parsing, it doesn't make sense to parse the args, this way we can avoid the configuration.

Thanks.

Did you mean for me to do this in a new PR, or will you be doing it upstream?

fabioxgn commented 5 years ago

@oddlid so just parse the command and remove the args parsing, it doesn't make sense to parse the args, this way we can avoid the configuration. Thanks.

Did you mean for me to do this in a new PR, or will you be doing it upstream?

Can you update this PR or open a new one please? Also, please, revert the import change to point to the main repo instead of your fork.

oddlid commented 5 years ago

@oddlid so just parse the command and remove the args parsing, it doesn't make sense to parse the args, this way we can avoid the configuration. Thanks.

Did you mean for me to do this in a new PR, or will you be doing it upstream?

Can you update this PR or open a new one please? Also, please, revert the import change to point to the main repo instead of your fork.

I've reverted my other changes in my fork now, so the only change from upstream is not using unidecode on argument parsing. Is this enough for you to pull my changes, or should I close this and create a new PR instead?

oddlid commented 5 years ago

Oops, seems I missed updating the tests for this. I'll fix.