Dice extensions - many #91
Conversation
…into dice-extensions
…nto dice-extensions
…nto dice-extensions
…nto dice-extensions
…nged func to store func infos to make evaluation easier, moved over to use Text where possible in evalShow and co, added good
…ole bunch of stuff
There was a problem hiding this comment.
I am approving this, since it passes all of my individual tests. However, I add a few caveats - I won't merge this until the end of the day in case you decide you want to spend some time improving these, but I'm also happy to merge now and issue these instead.
- I struggled to read some parts of your code due to the surrounding comments assuming a lot of knowledge. I brought up this issue already in my other comment - some kind of glossary may be helpful to avoid people having to keep too many terms in their head. Some explanation of the high-level structure of your code would also help - if you were to ask me to fix a bug right now, I would have genuinely no clue where to go. Finally, high-level explanations of some of your functions would be helpful - what does this function contribute to the implementation as a whole? (e.g. "this evaluates an inbuilt function on a list" or "this is used to evaluate the results of rolls that give numbers")
- The error handling is very mixed, and I found multiple places where you just got a
ParserExceptioninstead of something more helpful - e.g. pictured:
- The formatting of the output sometimes lacks code font - e.g. the second result in pictured:
This is still a very impressive implementation - I just cannot read it well enough to be confident that this is fully working code. Since this is entirely sandboxed (except for the inline command parsing, which I've read and am very happy with), I'm fine to merge since it won't break any other part of the bot - but for the sake of future maintainability it'd be nice to get the surrounding documentation and error handling improved.
| -- | This type class evaluates an item and returns a ListInteger (which is | ||
| -- either a value or values and their representations). | ||
| class IOEvalList a where | ||
| -- | Evaluate the given item into a ListInteger, possibly a string | ||
| -- representation of the value, and the number of RNG calls it took. If the | ||
| -- `a` value is a dice value, the values of the dice should be displayed. The | ||
| -- integer given initially is the current RNG count of the expression. This | ||
| -- function adds the current location to the exception callstack. | ||
| evalShowL :: PrettyShow a => RNGCount -> a -> IO (ListInteger, Maybe Text, RNGCount) | ||
| evalShowL rngCount a = catchBot (evalShowL' rngCount a) handleException | ||
| where | ||
| handleException (EvaluationException msg' locs) = throwBot (EvaluationException msg' (addIfNotIn locs)) | ||
| handleException e = throwBot e | ||
| pa = unpack $ prettyShow a | ||
| addIfNotIn locs = if null locs || pa /= Prelude.head locs then pa : locs else locs | ||
|
|
||
| evalShowL' :: PrettyShow a => RNGCount -> a -> IO (ListInteger, Maybe Text, RNGCount) |
There was a problem hiding this comment.
I'm going to be real with you, the comments here do not help me in the slightest. I think part of the issue is that there are just so many terms thrown around (ListInteger, RNG calls, and so on) which aren't supremely helpful without understanding the rest of the codebase. It might be useful to have an idea as to what IOEvalList does - yes, it evaluates an item and returns a ListInteger, but how is that used in the codebase? This question is about the context of the code, because I have no idea of the high-level structure of this.
I will still approve this, but I would quite like some more helpful commentary (or even a glossary of some kind) to make these comments easier to understand. There's no deadline on this, except I'd ideally like this fixed before you become less involved in the project so that other people are able to contribute to the dice part of the bot.
|
Comments have been greatly improved, thank you! Merging. |
Added inline rolling syntax and made it work with the NR stuff.
Added ways to make lists of values, as well as operate on them.
Moved stuff out of the single file so that different functionality is separated.
Commented stuff as well.
Added some new functions.
Split and genericised some things to make future work easier.
Covering stuff from the issue here #66.