DiceParser
DiceParser copied to clipboard
Rolisteam
Extract `parse()` function, and add unittest for Diceparser
Do not integrates thoses commit they are not ready, I am doing this PR because I need some help, and I thought it was the best way to discuss and let you see my code
Hello,
Since I started to do some modifications in DiceParser, I was never confident that I would not broke something. It's why I wanted to add unit tests for all operators. Furthurmore, I discovered a bunch of bugs (including core dumps) in the previous month witch could have been spotted by tests.
To add unit tests, the first step was to make the dies mockable (you can't unittest randomness!).
Then I extracted the parse() function to have a simple interface (you give it a string, you get back the result). This function will be called both from the cli (and other medium like discord, but I didn't do the modifications yet), and from my future unit tests. I tried my best to only move some part of the code without changing how it's working. Polishing will come when the unit tests will be working (and before the merge request).
After that I added unit tests. This add a dependency to Catch2. Technically I could have just copied one .h file (this library can be used as a single file, header-only library), but I think that by adding the submodule it should be easier to update Catch2 in the future. I mostly copy-pasted the CMakeList.txt file, and did not took the time to clean it up and will do before asking for the merge.
For debuging purpose, I added the -g flag in the CMakeList.txt files. When the PR will be ready, I will remove this commit.
The reason I am doing this PR is because I really don't understand how the operator evaluation is working (especially the role of the attribute set by the displayed() fonction. In the cli, it's working fine, but in the unit tests, I can't make it work for anything that include a sort, or witch return multiple values. I you have any clue on how to make the parse function working for the unittests, it would help me a lot.
Anyway, the more I look at the Diceparser core (the part that compute the dice result, not the interface like text/json/svg/discord/twitter/… output), the more I think it would be much easier to re-write it. However I don't want to throw the baby with the bath water (nor the bathroom or the plumbing either)! I definitively like the mini-language that you created, and the fact that you interact with lots of differents services (rolisteam/discord/cli/…), so what I would like to do it to create a drop-in replacement of the parsing and evaluating. It's why I extracted the parse() function (if I do the re-write, it's what I will change), and the unit tests (to validate that I didn't broke anything). That way, if you like my re-write (that I have yet to do), you could either chose to use it, or keep the current code but with a better code coverage.
I hope you didn't rolled a critical miss against this wall of text! If you don't have time to look at it, it's not really a problem, since I will be in vacation for 3 weeks in June without access to a computer, so I don't really expect to finish everything before end of July.
Rolistically, Robin. So my short term goals are:
- do what is needed to have a working
parse()function - have unit tests to validate the current behaviour of the different operators.
Then I think we could discuss on the interface of the parse() function. I think that returning a tree of results is probably the best, with some utility functions to convert it to text, handling color, and maybe other details that I don't see right know.
I took the time to read the whole text.
To be honest, I was thinking to replace API of diceparser to return JSON data. And let the client deal with it. I did not work about it but it seems to me a good solution because many languages support it. No complex data structure.
DiceParser is really good. It got some improvements and it does a really good job. But When I started its design I didn't know a lot of things about rpg system and so on. So the need of some feature asks for major change in its API. Some changes I made are ugly or not really safe. As it works well, I prefer dedicate my time to other part of Rolisteam. But I'm glad to add new features.
The compiler/interpretor/parsing part is using mechanism about my compilation course I had several years ago (as far i remember). When I had to add some new features or new mechanism it works like a charm. Even there are some ugly small changes but those changes are more tips that deep behavior. I agree about the need to improve it or rework it but I'm not sure the need to re-write it completely. If you have some point about it, I'll be glad to read them. What do you plan to do ?
I develop Rolisteam and all its component as simple as I can. The goal behind that is to make contribution easier. Some C++ powerful/useful component have really bad UX design. It isn't perfect but It is easy to read.
I will clone your repo on my personal account, then I will try to compile it and help you a bit with that.
I don't know which IDE you are using but you can ask cmake to compile in debug by giving it the right parameter. Qt Creator manages it for me but don't need to change the CMakefiles.txt. I know you will remove it.
Usually vim + all the unix tools, but for rolisteam I'm using QtCreator because I wasn't able to print the content of a QString in gdb (I didn't took much time to investigate). I'm also not really fluent with Cmake, so it was mostly a "make things works" patch. Thanks for telling me that QtCreator is smarter than I knew, I will look at it (and it's also probably the case for cmake).
DiceParser is really good.
I totally agree. I would not be working on this project if I didn't liked it in the first time!
As it works well, I prefer dedicate my time to other part of Rolisteam.
It's totally understandable, and you are totally right. Ideally I would like to be able to give you a proposition, were you could say "Yes I like it", and it would cost you no more work!
But I'm glad to add new features.
A saw that you were accepting PR, and really reactive. It is a pleasant experience for me.
The compiler/interpretor/parsing part is using mechanism about my compilation course I had several years ago (as far i remember). When I had to add some new features or new mechanism it works like a charm. Even there are some ugly small changes but those changes are more tips that deep behavior. I agree about the need to improve it or rework it but I'm not sure the need to re-write it completely. If you have some point about it, I'll be glad to read them. What do you plan to do ?
Currently I am testing stuff, but I should shortly start to put everything together. I will take 3 week of vacations without a computer, so it should be done in July.
What I would like is a clear API: you give a string in input and you get a structure in output. This structure should be easily convertible to and from json.
Internally, I want to have an explicit grammar (probably using PEGTL) instead of the current manual parsing. This should allow easier modification by being able to see the grammar itself.
And for the output structure, I am currently experimenting to see what need to be put inside, and what does not. Do you have any specification on your current json format? I didn't find it. Currently it is still changing too much, but as soon as I have something clean, I will share you the result.
Ok, It is true that using a tool to implement the grammar and get a syntax parser can be really useful. It could be a real step forward to manage some stuff.
In rolisteam, I use same mechanism (parsing by hand) to manage formula inside character sheet. It could be good to rewrite it with a dedicated tool.
PEGTL looks like good solution because it is cross-platform and it is header only. If you have to switch to another tool, we will have to make sure the next component is cross-platform and not a nightmare to integrate (such as boost).
So if you do the change it will be good if I can follow changes (even better take part of it) First, I want to be able to modify the code to add new features. Secondly, do the same change on formula
New feature : Several command at once
One feature which may be interesting to implement is a way to execute several time the same command.
My first draft about the syntax will be something like that: !3!2d6 equivalent of: 2d6 2d6 2d6
The goal for DiceParser is to return 3 results.
New feature : Detect double
Some games trigger some event when double value are rolled So, a new kind of validator which return true if there are double values could be great.
I have no clue about the syntax.
In rolisteam, I use same mechanism (parsing by hand) to manage formula inside character sheet. It could be good to rewrite it with a dedicated tool.
I will look at it once I will have finished to work on diceparser. Anyway, I planned to do some UI stuff to have a better experience while creating the character sheet. But later!
PEGTL looks like good solution because it is cross-platform and it is header only. If you have to switch to another tool, we will have to make sure the next component is cross-platform and not a nightmare to integrate (such as boost).
I get your concern.
So if you do the change it will be good if I can follow changes
Of course :)
even better take part of it
You are welcome to take part of it. I don't want to steal your time, but if you want to participate you are more than welcome. In your opinon what is the best way to exchange? With pull request to be able to easily comment a line? Or with another medium (mail, forum, reddit, …)?
First, I want to be able to modify the code to add new features.
At the moment just add the stuff you need on the current Diceparser (but don't implement my ideas unless you want them now, I'll do it in my v2), and I will integrate them in my re-write later (if my re-write is successful). Just keep me in touch so I will consider them when doing the design.
New feature : Several command at once New feature : Detect double
Nice ideas.We may use D for double but I fear that it may break user experience (even if lowercase d is still available to throw dices). Maybe S for same?
By the way, what is you politic about depreciation? If the e (explode) operator had only an optionnal validator (by default it would be the maximum value of the dice), then we could remove the K (explode and keep) to make space for newer operators. K would be replaced by ek (explode without validator then keep). And ideally we could make kl just K to simplify the grammar (since all operators would be single letters).
I fixed the mock.
12d100g1 should give 12 (since each die will create a new group), but it dosen't work, I am really surprised. I tested with an unmodified version of Diceparser, I was really surprised.
I pushed my draft of my re-write on gitlab. For the moment, I didn't implemented the parsing, I was mostly playing with the output structure. It is still in an early draft phase (no clang-format, single file, no build toolchain, lots of refactoring to do, …). The only dependency is catch2 (in a submodule).
I implemented an algorithm for group witch is working for all the cases I found relevant (you can see them in the unit tests at the end of the file). If you want to run them, compile interface2.cpp, and execute the generated binary (with "-h" to get the options, you may be interested by "-s").
I see that you are using a lot of C++ specific/new stuffs: template, lambda, etc.. So you will probably reduce the code line count by a huge amount but anyone who wants to contribute will need high level in C++ skills.
I always paid attention about this subject. I want python dev/java dev to be about to read the code and understand it. Even a non programmer may understand some stuff (if he can read english).
Other point, could you follow Rolisteam coding rules ? At least for naming stuff and curly brackets? no _ separator in fonction/variable name. Use uppercase letter: MyClassName, myFunctionName(), myVariableName(). It helps to separate std:: stuff and local stuff.
// please void function {
}
Clang format can do that for you. I plan to write my own configuration of clangformat to make it easier.
it is old and not fully respected but : http://wiki.rolisteam.org/index.php/CodingRules
To be tested: https://gist.github.com/obiwankennedy/1dca7a10787e61d0d83234724348cb2e
I see that you are using a lot of C++ specific/new stuffs: template, lambda, etc.. So you will probably reduce the code line count by a huge amount but anyone who wants to contribute will need high level in C++ skills.
The code definitively need some polish (I overused inline lambda and stuff like that, I need to split up the code in logical files, etc). I will do it when I will come back from vacation. My first goal was to have a proof of concept to be sure I could do what I want.
About the new features of c++, I totally disagree that I should not use them (unless their is a compiler compatibility issue). I'm using c++03 at work, and I can guarantee you that the code is much more complicated that if it used modern c++. And lambda are also used since java 8, and IIRC also in python ;) But I get your concern about readability. I know I need to add some comments, especially for the "g" operator to explain the general logic, and what each function does if the name isn't totally explicit. Also splitting the code in logical files would help.
Other point, could you follow Rolisteam coding rules ? At least for naming stuff and curly brackets? no _ separator in fonction/variable name. Use uppercase letter: MyClassName, myFunctionName(), myVariableName(). It helps to separate std:: stuff and local stuff.
Totally. Like I said I didn't took the time to set up clang format, so all the curly brace/indentation stuff will be fixed. About the naming, I'm not even consistent in my file and I will fix that (it's unusual for me to not be consistent on a coding standard).
Clang format can do that for you. I plan to write my own configuration of clangformat to make it easier. If you create one, I will gladly import it, otherwise I will probably use k&r style. IIRC it's the one I prefer.
it is old and not fully respected but : http://wiki.rolisteam.org/ index.php/CodingRules
Nice, I didn't saw it, I will follow it.