-
Notifications
You must be signed in to change notification settings - Fork 7
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 prompt string interpolation #311
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.
LGTM!
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.
LGTM
README.md
Outdated
|
||
#### Disabling the prompt text | ||
|
||
The prompt text can be disabled by setting it to an empty string in either the config or as a command line argument: |
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.
What does "disabled" mean in this context? ">" or ""?
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.
It means >
, which is why I specified "prompt text". If you have any ideas how to make it clearer, then I'll change it.
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.
This was changed to "prompt prefix text", which isn't great, but it should remove any ambiguity when combined with the example showing the result.
I think this is ready now? There are some basic unit test for IPv4 and IPv6, but they are not comprehensive at all. Should be fine though. |
One last thing we should consider taking a look at is moving the prompt configuration section in the README somewhere else, because it takes up a lot of space very early on for such a trivial feature. Edit: I hid it in a collapsible section that defaults to being closed. |
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.
LGTM!
Adds string interpolation support for the CLI prompt. In order to test this, the
get_prompt_message()
function has been extracted frommain()
.Furthermore, the parsing of command-line config arguments is now moved into the config itself. CLI args are now accessed through
MregCliConfig.get()
instead of viavars(args)
.