-
Notifications
You must be signed in to change notification settings - Fork 13
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
High Level API #92
base: main
Are you sure you want to change the base?
High Level API #92
Conversation
0e0391c
to
7a443bb
Compare
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.
Can you elaborate a bit more on the high-level principle of these Settings? What data is contained in which file, what files are exported, which are expected to be present on an external drive, etc?
Also in terms of code, I'm getting a bit confused because I regularly mix up Settings
(high-level API) and Setting
(binrw).
pub mod anlz; | ||
pub mod device; | ||
pub mod pdb; | ||
pub mod setting; | ||
pub mod util; | ||
pub(crate) mod xor; |
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.
Do we want to make the more internal modules private when they have a nicer API counterpart.
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.
Not sure. Maybe we should group the low-level stuff in a separate submodule or crate but leave it public for people who want to experiment or squeeze out maximum performance?
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.
@Holzhaus another layer makes sense. Basically a "core" library.
(hi gang, I've been watching the progress and though I would chime in. Hope it's not TOO weird to have me just pop into the conversation).
#[derive(Debug, Default, PartialEq, Eq, Clone)] | ||
pub struct Settings { |
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 we want the Default
to be? Just a struct with all None
s (which is derived) or with the actual default settings from rekordbox?
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.
Rekordbox' default settings probably make sense.
src/main.rs
Outdated
let mut export = DeviceExport::default(); | ||
export.load(path)?; | ||
let settings = export.get_settings(); |
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.
IMO constructing a mutable default and then loading from a file doesn't seem very rusty to me. Why not make load a constructor? Also allows for nice chaining.
let mut export = DeviceExport::default(); | |
export.load(path)?; | |
let settings = export.get_settings(); | |
let settings = DeviceExport::load(path)?.get_settings(); |
0679936
to
8a75859
Compare
a5eaf21
to
8a44d90
Compare
This is an attempt to look into a high-level API to make it easier to read from (and later also write to) Rekordbox device exports without knowing much about the internals.
As first step, I only added support for setting files:
The CLI makes use of the new API when calling the new
list-settings
subcommand:Based on #90 and #91.