cronokirby / alchemy

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

gen_parser #42

Closed nhooyr closed 7 years ago

nhooyr commented 7 years ago

Continuation of #32

Here is my current work so far: https://gist.github.com/nhooyr/969aabdca2476a13c777ac235f781c87

Rather than discuss in discord like we have lets move discussion here so that it is easy to track our conversation.

cronokirby commented 7 years ago

I don't like how this is reinventing the parser combinator. I feel like this isn't a very good path, because we'll need to reinvent a lot of things, instead of using the constructs elixir provides, such as pattern matching.

nhooyr commented 7 years ago

I don't understand what you mean. What has been reinvented? You would still need to pattern match the resulting arguments.

Splitting on spaces and quotes only is in my opinion too limiting. My friends that use my bot are not programmers and so they would never think that using quotes is necessary. So I find the :rest field very useful. It would get very boring to have to implement this along with regular splitting for every function I have that needs :rest with set_parser. Of course, I can use my implementation above but I mean to speak from the aspect of a user just starting to use this library. I don't think they should have to reinvent this themselves.

cronokirby commented 7 years ago

https://github.com/bitwalker/combine This is reinventing things like this. Writing a parsing engine is out of the scope of what alchemy should be doing, if you ask me.

nhooyr commented 7 years ago

I agree, it is definitely out of scope but what I've done is nowhere near as complicated or involved as the library you linked. My parser is very simple. Including something like combine would be crazy but I think the functionality provided by my parser is something very basic to a library like alchemy. You wrote cogs so that it would be easy and simple to define commands. This is in my opinion one more step in that direction, to make it easier and more simple to parse the arguments provided to those commands.

In addition, I don't see many cases where my parser will fall short of what a user wants and that they will not be able to easily integrate their use case using a custom function field. And if it is, they always have set_parser to use a completely custom parser. Even that library you linked does not support quoted arguments or a rest field. I think those two are part of the parsing needs of just about every bot. Of course we can provide quoted arguments without gen_parser but I don't think we can provide a rest field without gen_parser. My strong need for gen_parser is mostly just :rest. If there is a way to provide a rest field without gen_parser, then I am all for that solution and agree that gen_parser is extraneous.

cronokirby commented 7 years ago

You can use combine in combination with set_parser for implementing rest. (many(word())) If something is easily acheivable without having internal knowledge of the workings of the library, then there's little reason for us to provide an api for it, imo. Also I never suggested including it into alchemy. If the user feels they want a complex parser, then it's up to them to use combine if it fits their needs.

nhooyr commented 7 years ago

(many(word())) isn't the same as rest. (many(word())) is the same as the regex (\w+ ?)* but :rest is like .*.

While it may be easily achievable with another library, I think an out of the box solution would be better. I can easily see many bots using using a rest field. A user shouldn't have to rewrite that logic when we can abstract it away neatly. Also, I don't think gen_parser requires internal knowledge of the workings of the library.

I didn't think you suggested to include it, I was just giving an example of what I think would be extraneous in this situation.

This isn't a complex parser though, it's the bare minimum the library should support imo. If you still think its a bad idea, it's fine. Its not a big deal since I can always just use my parser myself. I just think it would be better and easier for new users to include it by default. I'll incorporate the quote parser in here to fix my errors in #32. #32 assumes that the first argument is not quoted, does not recognize args like somefoo"some foo bar"barfoo and does not support escaped quotes but this one fixes those issues.

cronokirby commented 7 years ago

Not everything potentially useful should be included into alchemy, especially when better solutions (use an actual parsing library) already exist.