-
Notifications
You must be signed in to change notification settings - Fork 25
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
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 PR! |
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"); |
Follow up #300 |
As discussed in #220, this PR adds an optional
export_keys
boolean param to thenative_db
macro 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: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.