Add option -m/--max #67
Conversation
There was a problem hiding this comment.
Thanks @donaldtn for this PR. @madduck, are you comfortable with this change? It introduces a --max option that crops any generated phrase to a desired max length.
I think, I could live with it, although a "select enough phrases until it is short enough" might have been more elegant and kept the only-whole-words paradigm of diceware. On the other hand it could easily lead into endless loops (for instance, cause no phrase with only 1 char could be created)
@donaldtn: before merging I would like to address some minor things:
- The --max default should be None or -1 in my opinion, cause zero is a perfectly valid value here.
- The tests currently do not test much more, than two trivial usecases. It would be nice if there could be shown, how phrases are really cropped. I commented the tests directly.
While (1) must be addressed (or you can give me better arguments, why 0 should mean something different than other numbers), (2) is not that important. I could do that myself afterwards.
What do you think? Would that be okay for you? Don't worry about the nitpicking, your contribution is appreciated!
| options = handle_options(['-m', '0']) | ||
| assert options.max == 0 | ||
| options = handle_options(['--max', '0']) | ||
| assert options.max == 0 |
There was a problem hiding this comment.
Values apart from 0 would be more interesting here.
| def test_get_passphrase_max(self): | ||
| options = handle_options(args=[]) | ||
| options.max = 15 | ||
| phrase = get_passphrase(options) | ||
| assert len(phrase) == 15 |
There was a problem hiding this comment.
This test would be more meaningful if it could ensure, that the original phrase was longer or shorter than 15 chars.
| @@ -362,4 +377,4 @@ def test_main_wordlist(self, argv_handler, capsys, wordlists_dir): | |||
| sys.argv = ['diceware', '-w', 'foo'] | |||
| main() | |||
| out, err = capsys.readouterr() | |||
| assert out == 'FooFooFooFooFooFoo\n' | |||
| assert out == 'FooFooFooFooFooFoo\n' | |||
There was a problem hiding this comment.
The trailing newline is nitpicking, I know.
| @@ -104,6 +104,9 @@ def handle_options(args): | |||
| parser.add_argument( | |||
| '-s', '--specials', default=0, type=int, metavar='NUM', | |||
| help="Insert NUM special chars into generated word.") | |||
| parser.add_argument( | |||
| '-m', '--max', default=0, type=int, metavar='NUM', | |||
There was a problem hiding this comment.
Default -1 or None would be more appropriate, I think.
| if options.max > 0: | ||
| result = result[:options.max] |
There was a problem hiding this comment.
Shouldn't truncating happen before inserting special chars (that might get lost this way round)?
|
Thanks, this is indeed the most pragmatic solution! |
#65
Saw this request and thought it would be useful as long as some websites limit the length of passwords.