feat: add support for types from ascii crate #255
Conversation
borsh/src/de/mod.rs
Outdated
|
|
||
| #[cfg(feature = "ascii")] | ||
| #[test] | ||
| fn test_ascii() { |
There was a problem hiding this comment.
please change logic and naming of tests:
test_ascii_string_to_ascii_string_roundtrip
test_ascii_str_to_ascii_string_roundtrip
And 2 separate tests for ascii/std-Strings interoperability (which are optional):
test_str_to_ascii_string_maybe_roundtrip
test_ascii_str_to_string_roundtrip.
There was a problem hiding this comment.
I will definitely not do that.
dj8yfo
changed the title
borsh/tests/test_strings.rs
Outdated
| test_string!(test_hello_10, "hello world!".repeat(30), true); | ||
| test_string!(test_hello_1000, "hello world!".repeat(1000), false); | ||
| test_string!(test_non_ascii, "💩", true); | ||
| test_string!(empty_string, "", true); |
There was a problem hiding this comment.
- please duplicate macro
test_string!withtest_ascii_string!, so that
there's a duplicate for eachtest_string!entry, besidestest_string!(non_ascii, "💩", true); - please move all
asciitests toborsh/tests/test_ascii_strings.rs(besidestest_string!(non_ascii, "💩", true);) - please move
asciicounterpart oftest_string!(non_ascii, "💩", true);toborsh/test/test_de_errors.rs, as mentioned in comment. - please split test, generated by
test_ascii_string!into 4 tests, mentioned in comment, 2 of which are not mandatory, asborshlibrary doesn't have to guarantee interoperability ofascii/stdstrings serializations, though illustrating this aspect is useful.
#![cfg(feature = "ascii")]
...
test_ascii_string!(a, "a", true);
test_ascii_string!(hello_world, "hello world", true);
test_ascii_string!(x_1024, "x".repeat(1024), true);
test_ascii_string!(x_4096, "x".repeat(4096), false);
test_ascii_string!(x_65535, "x".repeat(65535), false);
test_ascii_string!(hello_10, "hello world!".repeat(30), true);
test_ascii_string!(hello_1000, "hello world!".repeat(1000), false);
...The problem with proposed solution,
if let Ok(ascii_str) = ascii::AsciiStr::from_ascii(&value) {
...
} else {
..
}it doesn't set expectations on conversions from specific &str, whether they're gonna fail or not.
The tests should be along the lines of:
let ascii_str = ascii::AsciiStr::from_ascii($str).unwrap();There was a problem hiding this comment.
please move all ascii tests to borsh/tests/test_ascii_strings.rs
How is duplicating the same test_string/test_ascii_string invocations in two different files better than keeping it DRY in a single file? You’re asking me to duplicate code for no reason.
|
Address all but moving stuff to separate file and duplicating test cases. I’ve no intention of doing that ’cuz it’s silly. |
No description provided.