Move algorithms/randomness to crates #737

joshka · 2025-10-02T22:39:59Z

This takes a slightly different approach to the way that the web crate works by moving all rust code to proper rust files and checking if the code compiles in CI.

Related to but does not completely fix #711

Things to check before submitting a PR

the tests are passing locally with cargo xtask test all
- existing tests are broken
commits are squashed into one and rebased to latest master
PR contains correct "fixes #ISSUE_ID" clause to autoclose the issue on PR merge
- if issue does not exist consider creating it or remove the clause
non rendered items are in sorted order (links, reference, identifiers, Cargo.toml)
links to docs.rs have wildcard version https://docs.rs/tar/*/tar/struct.Entry.html
- latest instead
example has standard error handling
code identifiers in description are in hyperlinked backticks

[`Entry::unpack`]: https://docs.rs/tar/*/tar/struct.Entry.html#method.unpack

Things to do after submitting PR

check if CI is happy with your PR

joshka

Self review notes.

joshka · 2025-10-02T22:40:56Z

.github/workflows/mdbook-test.yml

        with:
-          cache: 'true'
+          cache: "true"
          toolchain: stable


This config is actually unnecessary as these are the default values

joshka · 2025-10-02T22:42:40Z

crates/algorithms/randomness/src/bin/choose.rs

+
+    let password: String = (0..PASSWORD_LEN)
+        .map(|_| {
+            let idx = rng.random_range(0..CHARSET.len());


Fixed for rand 0.9 (gen_range -> random_range).

Moving away from skeptic here highlights deprecated code like this easily with standard tooling

Praise: isolating our dependencies makes them much more deterministic.

joshka · 2025-10-02T22:45:33Z

crates/algorithms/randomness/src/bin/choose.rs

Note these files are in src/bin/. An alternative would be to put these in tests/ and have them actually run when running tests. This would require adding #[test] to each main function. I'm ambivalent as to which option is better with a slight preference for src/bin, but if running each example to ensure that it doesn't error when run has a higher precedence than I've assumed then tests/ with #[test] might be more convenient.

Praise: This makes the recipes and tests synchronized and there's no extra boiler plate

This is probably the main comment that I wanted feedback on. Do you want to run the code as part of the build, or is confirming that the code compiles enough?

The old rust adage of if it compiles it's correct is probably good enough for most things that would appear in the cookbook, but it could be reasonable to instead just write normal unit tests that show both the code and the expected output of things like this. I.e. instead of println!("...") in a main() function, we'd have have a test with an assertion that shows what's expected. This change would be useful for those that understand testing well, but it's a fairly significant difference to the way things are currently.

for web I wrote explicit tests, but they don't run on ci. There has to be a mix of things that run and things that don't we aren't going to get postgres deployed. I'd like some testing, optionally added.

joshka · 2025-10-02T22:46:21Z

crates/algorithms/randomness/src/bin/passwd.rs

+    let password: String = (0..PASSWORD_LEN)
+        .map(|_| rng.sample(Alphanumeric) as char)
+        .collect();


The example from where this was copied was not using Alphanumeric, but the text said it was. Fixed this in this change.

joshka · 2025-10-02T22:47:49Z

src/algorithms/randomness/rand-choose.md

-Randomly generates a string of given length ASCII characters with custom
-user-defined bytestring, with [`gen_range`].
+Randomly generates a string of given length ASCII characters with custom user-defined bytestring,
+with [`random_range`].


In general, I've rewrapped markdown at 100 chars to make it easier to read when editing. I'd recommend choosing a value (likely 100) and adding a markdownlint config file for this.

I'd accept a lint for it, I know the feeling about wanting to wrap lines.

joshka · 2025-10-02T22:51:08Z

Cargo.toml

+members = ["crates/algorithms/*", "crates/web"]
+
+[workspace.package]
+edition = "2024"


Going with the latest edition here for the new code. Thoughts / problems?

fingers crossed. I think its only an issue with skeptic

joshka · 2025-10-02T22:51:20Z

Cargo.toml

-license = "MIT/Apache-2.0"
+license = "MIT OR Apache-2.0"


This format was incorrect

Praise: Thank you for fixing that

joshka · 2025-10-02T22:52:27Z

Cargo.toml

+[workspace.dependencies]
+rand = "0.9"
+rand_distr = "0.5"


These won't yet affect the playground, but could reasonably easily by modifying the following code to include workspace deps:

https://github.com/rust-lang/rust-playground/blob/f8d7de52a3c139a0df4fe116bbbff19c30668c99/top-crates/src/lib.rs#L134-L161

joshka · 2025-10-02T22:53:38Z

Cargo.toml

+# NOTE: These dependencies add dependencies to the rust playground (play.rust-lang.org).
+# Be wary removing or changing these without considering this fact.
+# See https://github.com/rust-lang/rust-playground/blob/f8d7de52a3c139a0df4fe116bbbff19c30668c99/top-crates/src/lib.rs#L134-L161


Added this as it's tempting to remove the deps from here as they are not needed (rand / rand-distr are no longer included in the generated skeptic tests).

joshka · 2025-10-02T22:54:01Z

Cargo.toml

+# NOTE: These dependencies add dependencies to the rust playground (play.rust-lang.org).
+# Be wary removing or changing these without considering this fact.
+# See https://github.com/rust-lang/rust-playground/blob/f8d7de52a3c139a0df4fe116bbbff19c30668c99/top-crates/src/lib.rs#L134-L161
 [dependencies]


Re-sorted this section correctly

This takes a slightly different approach to the way that the web crate works by moving all rust code to proper rust files and checking if the code compiles in CI.

AndyGauge

I will see about not making ci required so we can make changes.

AndyGauge · 2025-10-03T19:49:20Z

crates/algorithms/randomness/src/bin/choose.rs

Praise: This makes the recipes and tests synchronized and there's no extra boiler plate

AndyGauge · 2025-10-03T19:57:33Z

crates/algorithms/randomness/src/bin/choose.rs

+
+    let password: String = (0..PASSWORD_LEN)
+        .map(|_| {
+            let idx = rng.random_range(0..CHARSET.len());


Praise: isolating our dependencies makes them much more deterministic.

AndyGauge · 2025-10-03T19:58:06Z

src/algorithms/randomness/rand-choose.md

-Randomly generates a string of given length ASCII characters with custom
-user-defined bytestring, with [`gen_range`].
+Randomly generates a string of given length ASCII characters with custom user-defined bytestring,
+with [`random_range`].


I'd accept a lint for it, I know the feeling about wanting to wrap lines.

joshka commented Oct 2, 2025

View reviewed changes

Move algorithms/randomness to crates

0fd7036

This takes a slightly different approach to the way that the web crate works by moving all rust code to proper rust files and checking if the code compiles in CI.

joshka force-pushed the randomness-to-crate branch from 6d9b545 to 0fd7036 Compare October 2, 2025 22:54

joshka mentioned this pull request Oct 2, 2025

Add tracing crate #736

Open

8 tasks

AndyGauge approved these changes Oct 3, 2025

View reviewed changes

AndyGauge merged commit 94c065e into rust-lang-nursery:master Oct 7, 2025
1 of 3 checks passed

Move algorithms/randomness to crates #737

Move algorithms/randomness to crates #737

Uh oh!

Conversation

joshka commented Oct 2, 2025

Things to check before submitting a PR

Things to do after submitting PR

Uh oh!

joshka 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyGauge 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!

Uh oh!

Uh oh!