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

Extract parse() function, and add unittest for Diceparser #15

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

Conversation

robinmoussu
Copy link

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.

Copy link
Member

@obiwankennedy obiwankennedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

But it could be cool to compile test when debug is enabled and forget them in release.

Die::ArithmeticOperator m_op;

std::mt19937 m_rng;
static std::shared_ptr<RandomGenerator> m_rng;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is useful to put a shared pointer on a static member. Both mechanisms have quiet the same goal.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are totally right, thanks for pointing it.

@@ -36,7 +36,7 @@ void FilterNode::run(ExecutionNode* previous)
{
if(m_validator->hasValid(tmp,m_eachValue))
{
Die* tmpdie = new Die();
Die* tmpdie = new Die(-1,-1); // yaaaaaarggl what does this block of code does?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All those code samples are doing the same thing, make a copy a die to do some work about this die without changing the original die.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so a copie constructor should do the trick. I will test.

@obiwankennedy
Copy link
Member

obiwankennedy commented Jun 7, 2018

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.

@robinmoussu
Copy link
Author

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).

@robinmoussu
Copy link
Author

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.

@obiwankennedy
Copy link
Member

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.

@robinmoussu
Copy link
Author

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).

@@ -105,6 +105,8 @@ TEST_CASE("group", "operator") {
CHECK( roll("4d10g10", {2,2,9,9}) == "2" );
CHECK( roll("4d10g10", {7,4,3,6}) == "2" );
CHECK( roll("4d10g10", {8,4,3,7}) == "2" );
CHECK( roll("12d100g100", {/* group A = 100 */ 50, 25, 25, /* group B = 100 */ 33, 34, 32, 1, /* group C = 100 */ 10, 11, 12, 13, 54}) == "3" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to debug it but G operator receive as previous result: 51, 51,51, 51,51, 51,51, 51,51, 51,51, 51.
The problem is there not in the g operator.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this I found the issue. I will push the fix tomorow.

The value generated by a random generator is scaled down to match the
interval of the desired value. I assumed it was using modulo, not divide,
to scale down, but I was wrong. So I added a parameter to the mock to
specify the range of the desired values. It's a quirk to have a scale
of 1, and thus no scaling.
@robinmoussu
Copy link
Author

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").

@obiwankennedy
Copy link
Member

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

@obiwankennedy
Copy link
Member

@robinmoussu
Copy link
Author

robinmoussu commented Jun 13, 2018 via email

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.

2 participants