-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add a Python command-line interface #441
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening a pull request to GNOLL! As soon as the core tests pass and someone approves your pull request, you'll be all ready to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. There's only one minor improvement to argparse i can see, but it's trivial, so there's no need to adjust it unless you want to.
Some other notes
- I'm just going to fix up some style deltas highlighted by the github action linter. A lot of these are a bit over-kill, but the style compliance on the repo is high so i'll make it fall in line.
- I see the javascript test is failing. It's definitely unrelated to this PR, but i'll make a note to file myself an issue to investigate (update: Various github actions are failing #442)
- Thanks for your feedback on documentation and the segfault - I will also log these to be investigated (Update Bugs raised in #441 #444)
Thanks for your contribution!
g.add_argument('--mock-const', metavar = 'N', type = int, default = 3, | ||
help = 'mocking constant') | ||
|
||
a = p.parse_args(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): You may be able to call this without the args object - I think argparse will automatically collect them for you - then you would be able to remove the parsing code
a = p.parse_args(args) | |
a = p.parse_args() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true in the usual case, but I figured that allowing parse_cmdline_args
to be called with an argument would be convenient for testing. Then you don't have to mock sys.argv
.
Signed-off-by: Ian Hunter <ian.hunter@intel.com>
Description
How Has This Been Tested
So far, just by trying it. If you like it, I can add real tests to this PR.
Unrelated, but I noticed that the syntax in the documentation doesn't match what is actually tested for rerolling (documented as
1d20r#1
, implemented as1d20r==1
) and macro calls (documented as#PLANETS
, implemented as@PLANETS
). Also,gnoll.roll('1d6', builtins = True, mock = 1)
segfaults.Change Type
Checklist