search

cronokirby / alchemy

A discord library for Elixir
MIT License
151 stars 34 forks source link

Cogs.CommandHandler: add proper argument parser #43

Closed nhooyr closed 7 years ago

nhooyr commented 7 years ago

This parser does not assume that the first element is unquoted, handles quoted arguments combined with non quoted characters and also supports escaping quotes within a quoted argument.

I also included a small test. In the future, I can break the single test into multiple tests to isolate the functionality tested but I don't think its necessary now.

nhooyr commented 7 years ago

Do you think I should change the name of the function from parse to split and the name of the module to ArgSplitter?

cronokirby commented 7 years ago

this parser is way too complex

nhooyr commented 7 years ago

The complexity is necessary for proper support of quoted arguments. I couldn't find a simpler way to accomplish it.

cronokirby commented 7 years ago

I feel like the concrete behaviour of this parser is too difficult to reason about for it to be a default. The combination of being able to quote things to get around splitting, and then being able to escape quotes, and then being able to escape escapes. As well as the fact that the implementation for this is really ugly, and also really slow, makes me feel that this isn't a sane default.

nhooyr commented 7 years ago

It doesn't support escaping escapes (slashes?) as thats not necessary. Only quotes can be escaped.

The implementation may be subjectievly ugly but why do you think it's slow?

In any case, I'll close this PR but we'll have to revert #32 because it was buggy.