Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

derive Key #239

Merged
merged 4 commits into from
Apr 9, 2022
Merged

derive Key #239

merged 4 commits into from
Apr 9, 2022

Conversation

ModProg
Copy link
Collaborator

@ModProg ModProg commented Apr 1, 2022

fixes #106

@ModProg
Copy link
Collaborator Author

ModProg commented Apr 1, 2022

@ecton It is probably a good idea, if you run cargo expand --test key and have a read of what the macro produces.

@ModProg
Copy link
Collaborator Author

ModProg commented Apr 1, 2022

also feel free to suggest some actual tests :D

@ecton
Copy link
Member

ecton commented Apr 2, 2022

Just wanted to say thank you for getting this submitted. I started feeling a little under the weather this afternoon, so I'll check this out soon but it'll be at least tomorrow.

@ecton
Copy link
Member

ecton commented Apr 3, 2022

This all looks great. As for real tests, I would look at #240 for example of two values being encoded that produces an interesting edge case and uses two datatypes. I wouldn't worry too much about thorough testing -- the main test the macro needs to worry about it to ensure every bit of data is encoded. The testing of the composite key encoding/decoding can be handled independently, as long as we are confident in the unit tests calling the encoding/decoding functions in the proper order.

There are some changes in main that hopefully will clean up some of the ugliness from the return types in decode_composite_key. I've deprecated encode/decode_composite_key in favor of two structs: CompositeKeyEncoder, CompositeKeyDecoder. An example usage:

 let value1 = String::from("hello");
 let value2 = 42_u32;
 let mut encoder = CompositeKeyEncoder::default();
 encoder.encode(&value1).unwrap();
 encoder.encode(&value2).unwrap();
 let encoded = encoder.finish();
 
 let mut decoder = CompositeKeyDecoder::new(&encoded);
 let decoded_string = decoder.decode::<String>().unwrap();
 assert_eq!(decoded_string, value1);
 let decoded_u32 = decoder.decode::<u32>().unwrap();
 assert_eq!(decoded_u32, value2);
 decoder.finish().expect("trailing bytes");

The last thing is to add one more option to the derive macro: allow_null_bytes = true. By default this should be false. When this is set, call CompositeKeyEncoder::allow_null_bytes_in_variable_fields. This relaxes a restriction that might be irrelevant for many users, but should be enabled by default to avoid the appearance of bugs.

@ModProg
Copy link
Collaborator Author

ModProg commented Apr 9, 2022

@ecton I added the new encoder/decoder and some test.

@ecton ecton merged commit 0bc4ce6 into khonsulabs:main Apr 9, 2022
@ecton
Copy link
Member

ecton commented Apr 9, 2022

Thank you as always @ModProg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deriveable support for structs as View keys
2 participants