Feature cleanup #31
Feature cleanup #31
Conversation
knickish
force-pushed
the
feature_cleanup
branch
from
7d86e52
to
145f0ce
Compare
There was a problem hiding this comment.
There are still a lot of changes in this PR. I started reviewing some of the files and added a few comments. In general - I'd like to avoid as much as possible adding #[cfg(...)] everywhere - it makes the code much more complex, harder to read and harder to maintain. Let's think how to simplify it?
| @@ -23,7 +23,7 @@ impl<'a> PickleDbListExtender<'a> { | |||
| /// | |||
| /// # Examples | |||
| /// | |||
| /// ```no_run | |||
| /// ```ignore | |||
There was a problem hiding this comment.
why change no_run to ignore?
no_run just doesn't run the code, but ignore doesn't even build it 🤔
| @@ -6,6 +6,7 @@ pub struct TestResources { | |||
| } | |||
|
|
|||
| impl TestResources { | |||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
Do you know why this is needed? We do use ommon::TestResources::new(...) 🤔
| assert!(db.lget::<Vec<i32>>("list1", 0).is_none()); | ||
|
|
||
| if let SerializationMethod::Yaml = ser_method!(ser_method_int) { | ||
| { |
There was a problem hiding this comment.
Why add those extra curly brackets?
| @@ -239,6 +277,7 @@ fn override_lists(ser_method_int: i32) { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(all(feature = "bincode", feature = "yaml"))] | |||
There was a problem hiding this comment.
why limiting this test to only bincode and yaml?
| @@ -1,16 +1,48 @@ | |||
| #![allow(clippy::float_cmp)] | |||
|
|
|||
| use pickledb::{PickleDb, PickleDbDumpPolicy, SerializationMethod}; | |||
| #[cfg(any( | |||
There was a problem hiding this comment.
I must admit I don't really like adding the supported configuration to every test, it significantly bloats the code and will make adding a new configuration require tons of changes. Unless it's absolutely necessary I'd prefer to avoid it
| #[derive(Serialize, Deserialize, Debug)] | ||
| struct Coor { | ||
| x: i32, | ||
| y: i32, | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug)] | ||
| struct MySquare { | ||
| x: u32, | ||
| } |
There was a problem hiding this comment.
Why extract these structs outside of the test case?
|
Ok, I couldn't come up with a better way at the time. I need nanoserde for my project so I'll just use my fork for now, and upstream it if I come up with a cleaner way. |
ok sure, please feel free to open another PR at a later time. Should we close this one and #30 or do you prefer to keep them open? |
|
I'll go ahead and close them, and just make a new one if I come up with a cleaner way to support alternative serialization libs. Thanks! |
This branch just does the work related to allowing the feature flags to actually be used. Currently only some combinations will work, this pr makes it so that any combination of the serialization types may be used.