Issue287: create random password from a set of allowed characters #372

anna-liao · 2017-11-27T18:21:53Z

I categorized this example under text processing. Do you think that is the appropriate category?

ludwigpacifici · 2017-11-27T22:15:52Z

src/basics.md

+
+[![rand-badge]][rand] [![cat-text-processing-badge]][cat-text-processing]
+
+Randomly generates a string of given length ASCII characters in the range `A-Z, a-z, 0-9`.


Can you give more details, such as gen_ascii_chars is the method which a hardcoded range of character.

Plus, I would show the choose method which is a user defined set of allowed characters. Maybe wait for the green flag of @budziq for this one.

Agreed on both arguments:

lets add links to key identifiers in the description.

I'd suggest to add two snippets here the second one being an iterator based in choose like the one used to implement gen_ascii_chars but taking arbitrary bytestring in the constructor, then used in identical fashion as the first trivial snippet. That would be very cool!

budziq

few suggestions

budziq · 2017-11-28T08:53:04Z

src/basics.md

 | [Generate random numbers within a range][ex-rand-range] | [![rand-badge]][rand] | [![cat-science-badge]][cat-science] |
 | [Generate random numbers with given distribution][ex-rand-dist] | [![rand-badge]][rand] | [![cat-science-badge]][cat-science] |
 | [Generate random values of a custom type][ex-rand-custom] | [![rand-badge]][rand] | [![cat-science-badge]][cat-science] |
+| [Create random passwords from a set of allowed characters][ex-rand-passwd] | [![rand-badge]][rand] | [![cat-text-processing-badge]][cat-text-processing] |


I'd remove the "text-processing" category. We are not really doing any processing of input text.

budziq · 2017-11-28T08:56:00Z

src/basics.md

+use rand::{thread_rng, Rng};
+
+fn main() {
+    let s: String =


lets run rustfmt on the snippet

budziq · 2017-11-28T09:00:24Z

src/basics.md

+
+[![rand-badge]][rand] [![cat-text-processing-badge]][cat-text-processing]
+
+Randomly generates a string of given length ASCII characters in the range `A-Z, a-z, 0-9`.


Agreed on both arguments:

lets add links to key identifiers in the description.

I'd suggest to add two snippets here the second one being an iterator based in choose like the one used to implement gen_ascii_chars but taking arbitrary bytestring in the constructor, then used in identical fashion as the first trivial snippet. That would be very cool!

anna-liao · 2017-12-03T21:14:29Z

r? @budziq @ludwigpacifici I added an example with choose. Was hoping to add punctuation into the bytestring, but the function call (commented out with //) doesn't work.

budziq

Thanks @anna-liao ! few suggestions below:

I would split this example into two separate code snippets.

budziq · 2017-12-06T11:18:33Z

src/basics.md

 [`BufRead`]: https://doc.rust-lang.org/std/io/trait.BufRead.html
 [`BufReader`]: https://doc.rust-lang.org/std/io/struct.BufReader.html
 [`chain_err`]: https://docs.rs/error-chain/*/error_chain/index.html#chaining-errors
+[`choose`]: https://docs.rs/rand/0.3.18/rand/trait.Rng.html#method.choose


lets use wildcard version numbers

budziq · 2017-12-06T11:18:49Z

src/basics.md

 [`File::try_clone`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.try_clone
 [`File`]: https://doc.rust-lang.org/std/fs/struct.File.html
 [`foreign_links`]: https://docs.rs/error-chain/*/error_chain/#foreign-links
+[`gen_ascii_chars`]: https://docs.rs/rand/0.3.18/rand/trait.Rng.html#method.gen_ascii_chars


lets use wildcard version numbers

budziq · 2017-12-06T11:20:25Z

src/basics.md

+    let s: String = thread_rng().gen_ascii_chars().take(30).collect();
+    println!("{}", s);
+
+    let alphanum_bytestring =


I'd suggest implementing an iterator type like the AsciiGenerator instead that would take an rng in a constructor and for added merrit a "charset" (a hardcoded bytestr for our purposes without anych dynamic generation with ranges and chain) as an argument.

Then it would be used almost identically as the gen_ascii_chars

budziq · 2017-12-06T11:38:25Z

src/basics.md

+
+    let mut rand_bytevec: Vec<u8> = Vec::new();
+
+    for _ in 0..30 {


If the iterator approach would be used, pretty much all of this code would be not needed.

budziq · 2017-12-06T11:40:40Z

src/basics.md

 | [Generate random numbers within a range][ex-rand-range] | [![rand-badge]][rand] | [![cat-science-badge]][cat-science] |
 | [Generate random numbers with given distribution][ex-rand-dist] | [![rand-badge]][rand] | [![cat-science-badge]][cat-science] |
 | [Generate random values of a custom type][ex-rand-custom] | [![rand-badge]][rand] | [![cat-science-badge]][cat-science] |
+| [Create random passwords from a set of allowed characters][ex-rand-passwd] | [![rand-badge]][rand] | |


there should be a category here

budziq · 2017-12-06T11:40:45Z

src/intro.md

 | [Generate random numbers within a range][ex-rand-range] | [![rand-badge]][rand] | [![cat-science-badge]][cat-science] |
 | [Generate random numbers with given distribution][ex-rand-dist] | [![rand-badge]][rand] | [![cat-science-badge]][cat-science] |
 | [Generate random values of a custom type][ex-rand-custom] | [![rand-badge]][rand] | [![cat-science-badge]][cat-science] |
+| [Create random passwords from a set of allowed characters][ex-rand-passwd] | [![rand-badge]][rand] | |


there should be a category here

budziq · 2017-12-06T11:49:01Z

src/basics.md

+        String::from_utf8((b'a'..b'z')
+        .chain(b'A'..b'Z')
+        .chain(b'0'..b'9')
+        //.chain([b'!', b'?', b'%'])


It does not work because ranges give you u8 while iterating over slice gives you &u8 so the solution (if we wanted to go with this approach and we do not ;)) would be to clone the iterator contents.
.chain([b'!', b'?', b'%'].iter().cloned()) or even better .chain(b"!?%".iter().cloned()).

On the other hand I would suggest to just go with bytestring literal

const CHARSET: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZ\ abcdefghijklmnopqrstuvwxyz\ 0123456789)(*&^%$#@!~";

anna-liao · 2017-12-06T16:42:55Z

@budziq Thanks for the review. I revised the PR based on your suggestions. With the choose example, if I manually compose the vec it seems to work, but I am having issues writing an interator like for AsciiGenerator. Can you provide some guidance on how to write CustomGenerator to work properly?

budziq

@anna-liao few suggestions below :)

budziq · 2017-12-06T20:47:57Z

src/basics.md

+use rand::{thread_rng, Rng};
+
+fn main() {
+    let s: String = thread_rng().gen_ascii_chars().take(30).collect();


lets change the variable name to a more descriptive one.

budziq · 2017-12-06T20:49:16Z

src/basics.md

+    let mut s = String::new();
+
+    for _ in 0..30 {
+        s.push(*thread_rng().choose(CHARSET).unwrap() as char);


instead of for loop you can use a relatively simpler functional appproach without a need for manual string append. Also obtaining a thread_rng might be relatively costly so lets reuse one instance througth the generation process.

let mut rng = thread_rng(); let password: String = (0..30) .map(|_| *rng.choose(CHARSET).unwrap() as char) .collect();

This version might be more readable and concise than separate type approach with CustomGenerator so I'll leave it up to you to chose one.

budziq · 2017-12-06T21:06:58Z

src/basics.md

+    }
+}
+
+fn gen_rand_chars<'a>(&'a mut self) -> CustomGenerator<'a, Self> where Self: Sized {


I guess that you are trying to emulate the gen_ascii_chars method here but it is not possible to extend the Rng in such way. All we can do is prepare a standard constructor that will take an rng and a charset.

Some along the lines of

impl<'a, R> CustomGenerator<'a, R> { fn new(rng: R, charset: &'a [u8]) -> Result<Self> { if charset.len() > 0 { Some(CustomGenerator { rng: rng, charset: charset, }) } else { Err(ErrorKind::InvalidCharset.into()) } } }

budziq · 2017-12-06T21:07:47Z

src/basics.md

+use rand::{thread_rng, Rng};
+
+struct CustomGenerator<'a, R:'a> {
+    rng: &'a mut R,


I'd probably go with owing the rng to make the lifetimes simpler. But I'll leave it up to you :)
We might go with having a refference to some outsude charset

anna-liao · 2017-12-07T20:00:28Z

@budziq Thanks for the help! I have learned more about idiomatic Rust syntax. I decided to stick with the non-generator case for user-specified charset. I couldn't get the generator case working. I think the generator case would be good as an additional example?

budziq

Minor suggestions in a review

I couldn't get the generator case working.

No problem it is a bit verbose anyway

budziq · 2017-12-08T07:31:15Z

src/basics.md

+
+    let mut rng = thread_rng();
+    let password: String = (0..30)
+        .map(|_| *rng.choose(CHARSET).unwrap() as char)


we generally avoid unwrap in the examples, we should either document why the unwrap will be always ok as long as the CHARSET is nonempty or provide a generalized solution like:

let password: Option<String> = (0..30) .map(|_| Some(*rng.choose(CHARSET)? as char)) .collect(); println!("{:?}", password);

@budziq Thanks for showing me how to implement the generator case! I have updated the PR as suggested to remove the unwrap() and print an Option instead.

ludwigpacifici reviewed Nov 27, 2017

View reviewed changes

budziq suggested changes Nov 28, 2017

View reviewed changes

budziq suggested changes Dec 6, 2017

View reviewed changes

budziq suggested changes Dec 8, 2017

View reviewed changes

create random passwords from a set of allowed characters

c1b8a31

budziq merged commit 5da134d into rust-lang-nursery:master Dec 12, 2017

anna-liao deleted the issue287 branch December 12, 2017 20:59


		[![rand-badge]][rand] [![cat-text-processing-badge]][cat-text-processing]

		Randomly generates a string of given length ASCII characters in the range `A-Z, a-z, 0-9`.

Issue287: create random password from a set of allowed characters #372

Issue287: create random password from a set of allowed characters #372

Uh oh!

Conversation

anna-liao commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

budziq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-liao commented Dec 3, 2017

Uh oh!

budziq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-liao commented Dec 6, 2017

Uh oh!

budziq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-liao commented Dec 7, 2017

Uh oh!

budziq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anna-liao commented Nov 27, 2017 •

edited

Loading