Generate unique device ID on native using rust's crates, as suggested in D4616.
Details
Diff Detail
- Repository
- rCOMM Comm
- Branch
- inka/deviceID_rs
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
.gitignore | ||
---|---|---|
11 | This was the only reasonable place I found for now. I figured once we start using the services there will be some subfolder handling communication with them, and this code could then probably be moved there. I could create such a subfolder now, but it seemed like an overkill, and I don't really know where it will need to be placed. |
native/cpp/CommonCpp/CryptoTools/rust-utils/Cargo.toml | ||
---|---|---|
10 | Since regex follows semver, "1" would be correct in the version range. But upstream seems to recommend a major minor version. | |
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs | ||
29–58 | Could use something like https://github.com/BurntSushi/quickcheck to dynamically generate input. |
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs | ||
---|---|---|
29–58 | I don't understand what exactly you want to generate... We have three possible values of the input so... |
native/cpp/CommonCpp/CryptoTools/rust-utils/Cargo.toml | ||
---|---|---|
10 | I think we should just specify major and minor versions, so 0.8 and 1.6 | |
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs | ||
5 | I think this should just be DeviceType | |
33 | nit: I think it's helpful to prefix tests with the function they're testing. so in this case, something like generate_device_id_ks would be better |
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs | ||
---|---|---|
17–21 ↗ | (On Diff #15466) | It's ok now, but we can reduce the duplication a little bit more by determining prefix separately |
32 ↗ | (On Diff #15466) | Printing all the cases is noisy. We should print the result in case of failure: to do that you can use the second and subsequent parameters of assert!. |
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs | ||
---|---|---|
32 ↗ | (On Diff #15466) | Normally the output of test that have passed is captured anyway. To see the output of non-failed tests we need to explicitly run cargo test -- --nocapture. |
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs | ||
---|---|---|
40 ↗ | (On Diff #15467) | I know I could have extracted that as well, but I think it's more clear if test phases (preparation (nothing to do in this case), execution, assertion) are more or less visible in the test itself. |
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs | ||
---|---|---|
53 ↗ | (On Diff #15467) | moble -> mobile |
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs | ||
---|---|---|
17–21 ↗ | (On Diff #15467) | Why not just call Alphanumeric.sample_string()? Example here: https://github.com/CommE2E/comm/blob/master/services/identity/src/token.rs#L35 |
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs | ||
---|---|---|
40 ↗ | (On Diff #15467) | You can say that result: {} does not match regex {} so that the message is less ambiguous |
native/native_rust_library/src/crypto_tools.rs | ||
---|---|---|
17 ↗ | (On Diff #17444) |
-inka |
17 ↗ | (On Diff #17444) |
-Marcin
-inka |
native/native_rust_library/src/lib.rs | ||
37–41 ↗ | (On Diff #17444) |
-Marcin |
81–82 ↗ | (On Diff #17444) |
-Marcin, before // Crypto Tools was added |
native/native_rust_library/src/crypto_tools.rs | ||
---|---|---|
26–29 ↗ | (On Diff #17444) | Because we are using cxx we now need a wildcard for the enum. Context: cxx.rs/shared |
Adding @marcin because he reviewed D5300, which changes have been moved here.
@marcin Can you please take a look at this inline, and tell me if you still want this changed?
native/native_rust_library/src/crypto_tools.rs | ||
---|---|---|
27–28 ↗ | (On Diff #17445) | Are you sure we need to call the macro and return an Err? I guess that returning should be enough, as the caller may decide what to do with Result |
42–43 ↗ | (On Diff #17445) | Can we find a different name for the argument? It feels like using type name might be confusing. |
native/native_rust_library/src/crypto_tools.rs | ||
---|---|---|
27–28 ↗ | (On Diff #17445) | I looked into it and I think we actually don't need this, sorry |
native/native_rust_library/src/crypto_tools.rs | ||
---|---|---|
40–41 ↗ | (On Diff #17510) | I renamed the variable whose only characteristic in this context is being a string to s based on function declarations in std::string::String |