Add export_keys optional param to native_db macro
#246
Conversation
This adds an optional `export_keys` bool param to the `native_db` proc macro that controls the visibility of the model's secondary key enum. Default value is `false`, which matches the existing behaviour of setting the visibility of the secondary key enum to `pub(crate)`. Using `#[native_db(export_keys = true)]` changes this to `pub`. See vincent-herlemont#220 for discussion.
|
@Grimeh Thank you for your MR! |
|
@Grimeh Can you rename your commits following the conventional commits standard? |
| use native_model::{native_model, Model}; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| // TODO somehow test visibility of keys enum from a sibling/child crate? |
There was a problem hiding this comment.
I have left a TODO in tests/macro_def/export_keys_attribute.rs:5 to note that this functionality can't be fully tested without somehow testing from an external crate. I could just remove that TODO if you're happy with how it is, not sure what your thoughts on the matter are.
IMHO, it is preferable for this feature to be tested by an external library to avoid regressions. For now, it's fine, but I would prefer that you leave the TODO.
| /// Macro which link [`native_model`](https://crates.io/crates/native_model) to the Native DB. See | ||
| /// [`Builder.define`](struct.Builder.html#method.define) for more information. | ||
| /// | ||
| /// Optional parameter `export_keys` controls whether the model keys enum is visible outside of the | ||
| /// crate, default value is false with visibility limited to `pub(crate)`. Use | ||
| /// `#[native_db(export_keys = true)]` to make enum visible outside of crate (ie. `pub`). |
There was a problem hiding this comment.
What do you think about moving the documentation of export_keys into the documentation of define in order to have all the model creation documentation in one place?
| let item = Item { | ||
| id: 1, | ||
| name: "test".to_string(), | ||
| }; | ||
|
|
||
| let key = item.native_db_primary_key(); | ||
| assert_eq!(key, 1_u32.to_key()); |
There was a problem hiding this comment.
I suggest simply testing whether the database works with a get on the primary keys and a get on the secondary keys. This ensures that there are no regressions with the addition of the new export_keys parameter.
| let item = Item { | |
| id: 1, | |
| name: "test".to_string(), | |
| }; | |
| let key = item.native_db_primary_key(); | |
| assert_eq!(key, 1_u32.to_key()); | |
| let mut models: Models = Models::new(); | |
| models.define::<Item>().unwrap(); | |
| let db = Builder::new() | |
| .create_in_memory(&models) | |
| .unwrap(); | |
| let rw = db.rw_transaction().unwrap(); | |
| rw.insert(Item { id: 1, name: "test".to_string() }).unwrap(); | |
| rw.commit().unwrap(); | |
| // Get primary key | |
| let r = db.r_transaction().unwrap(); | |
| let result_item: Item = r.get().primary(1u32).unwrap().unwrap(); | |
| assert_eq!(result_item.id, 1); | |
| assert_eq!(result_item.name, "test"); | |
| // Get secondary key | |
| let r = db.r_transaction().unwrap(); | |
| let result_item: Vec<Item> = r.scan().secondary(ItemKey::name).unwrap().all().unwrap().try_collect().unwrap(); | |
| assert_eq!(result_item.len(), 1); | |
| assert_eq!(result_item[0].id, 1); | |
| assert_eq!(result_item[0].name, "test"); |
As discussed in #220, this PR adds an optional
export_keysboolean param to thenative_dbmacro which allows for table keys to be made public and thus externally accessible from the crate.I have left a TODO in
tests/macro_def/export_keys_attribute.rs:5to note that this functionality can't be fully tested without somehow testing from an external crate. I could just remove that TODO if you're happy with how it is, not sure what your thoughts on the matter are.