-
-
Notifications
You must be signed in to change notification settings - Fork 120
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
Global config store #267
Global config store #267
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,7 @@ use clap::{Parser, Subcommand}; | |
use derive_more::Display; | ||
use gosub_engine::config; | ||
use gosub_engine::config::settings::Setting; | ||
use gosub_engine::config::storage::json::JsonStorageAdapter; | ||
use gosub_engine::config::storage::sqlite::SqliteStorageAdapter; | ||
use gosub_engine::config::storage::*; | ||
use gosub_engine::config::StorageAdapter; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name "store" was a bit ambiguous. They are actually storage adapters since we already have a config store. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it would make sense to reexport the StorageAdapters in use gosub_engine::config::storage::* Then you could also make your There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Sharktheone Can you guve an example on what you mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, doing this in mod json;
mod memory;
mod sqlite;
pub use json::*;
pub use memory::*;
pub use sqlite::*; Then you can do this use gosub_engine::config::storage::* and still use |
||
use std::str::FromStr; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,7 +88,7 @@ impl Setting { | |
|
||
match self { | ||
Setting::Map(values) => values.clone(), | ||
_ => Vec::new(), | ||
other => vec![other.to_string()], | ||
} | ||
} | ||
} | ||
|
@@ -155,7 +155,7 @@ impl FromStr for Setting { | |
|
||
/// Converts a string to a setting or None when the string is invalid | ||
fn from_str(key: &str) -> Result<Setting> { | ||
let (key_type, key_value) = key.split_once(':').unwrap(); | ||
let (key_type, key_value) = key.split_once(':').expect(""); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something that says, what has failed in the expect would be nice, else it is more or less the same as |
||
|
||
let setting = match key_type { | ||
"b" => Setting::Bool( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
pub mod json; | ||
pub mod memory; | ||
pub mod sqlite; | ||
mod json; | ||
mod memory; | ||
mod sqlite; | ||
|
||
pub use json::*; | ||
pub use memory::*; | ||
pub use sqlite::*; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,10 +164,7 @@ mod test { | |
#[test] | ||
fn resolver() { | ||
// Add simple logger, if not possible, that's fine too | ||
match SimpleLogger::new().init() { | ||
Ok(_) => {} | ||
Err(_) => {} | ||
} | ||
let _ = SimpleLogger::new().init(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When i think about this, I'm not sure if we want to to this here. It maybe would make sense to use the |
||
|
||
let mut dns = Dns::new(); | ||
|
||
|
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.
You have a
-
in the filename, I've seen this also in the other files in the bin directory now. Everywhere we have underscores and not dashes in the file names. I guess going for consistency is probably good. I'd generally avoid dashes/spaces and so on in file names, I just use a underscore for it.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 personally like dashes in binaries and options :)
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.
Same here — I prefer dashes to underscores in binary executable names. E.g., in Linux, the executable name is
google-chrome
. (For binary executables, even better is a short name with no dashes or underscores.)