-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix for Issue #2 in cro-ki/xdice #3
Conversation
* Gave Score a __str__ method so expressions would construct properly (formerly the string constructed was using __repr__ which gives a full class representation but is not appropriate for `eval`) * Updated roll.py to use argparse (mainly for my own sanity.)
Alright for the argparse use,. As said in the issue tread, the str addition is for compat with python3.8+ |
Also, I restored the
The numeric-only is:
and the verbose output is:
|
At last, since you allowed one on more expressions in the CLI, I changed the main method to allow the user to eval a few expressions in one row (ex: |
Oh no sweat on the argparse thing. I guess I hadn't noticed the difference between the two more verbose options so sorry about that. I half expected you to kick it back due to the roll.py alteration :D Only other thing I can think left to do is that I noticed your version on GitHub was 1.2 and pip was installing 1.1.4. That's why there's that initial comment about the error lines not lining up. As soon as I uninstalled the 1.1.4 version and cloned the repository everything lined up again. So you might consider updating the pip distribution once you are wholly satisfied that it's ready for the wider world. |
You're right, I did'nt published it so far on pypi, it's done. Thanks! |
Well when you get a series of inputs from argparse like that, it's a list of strings. i joined with no whitespace just as a matter of habit. I thought you stripped whitespace before sending to eval so it didn't trip any mental warning bells. If it'll do multiple groups of dice rolls, then you'd want to join with whitespace I suppose. |
Well, the 2 options seems good to me:
I choose the second one for now, but I'm still hesitating... |
Well I'll leave that up to you. From my perspective, the CLI application is primarily for testing and experimenting. I know personally I'm putting xdice into a Discord bot (we needed something to handle some casual dice rolling for Blood Bowl) and the command line tool is just to check certain things out. So whether the CLI takes multiple expressions or not doesn't bother much all that much. Out of habit, I tend to put spaces around mathematical operators, so my own personal leaning would be to just do a single expression and handle whitespace without quotes, but entirely up to you. Having the library functional was what prompted the entire investigation. I did notice you switched that to |
__str__
method so expressions would construct properly (formerly the string constructed was using__repr__
which gives a full class representation but is not appropriate foreval
)