move config to separate module#176
Conversation
|
Hi @nikitos , please help resolve the conflicts. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors configuration handling by extracting the config module from the main application into a separate library crate. The purpose is to avoid circular dependencies when using HlsConfig struct in the HLS protocol implementation.
- Creates a new
library/configcrate with its own Cargo.toml, README, and CHANGELOG - Removes config module from the main application and adds dependency on the new config library
- Updates imports to use the external config crate instead of the internal module
Reviewed Changes
Copilot reviewed 6 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| library/config/README.md | Initial README for the new config library |
| library/config/Cargo.toml | Package manifest for the config library crate |
| library/config/CHANGELOG.md | Changelog file for the config library |
| application/xiu/src/main.rs | Updates import to use external config crate |
| application/xiu/src/lib.rs | Removes internal config module and adds external config dependency |
| application/xiu/Cargo.toml | Removes config-related dependencies and adds config library dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 15 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| toml = "0.5.8" | ||
| serde_derive = "1.0" | ||
| serde = { version = "1.0.101", optional = true, features = ["derive"] } | ||
| failure = "0.1.8" |
There was a problem hiding this comment.
The failure crate is deprecated and unmaintained. Consider replacing it with anyhow for error propagation and thiserror for defining error types (e.g., remove this line and add thiserror = "1"; if you need context-rich errors, also add anyhow = "1").
| failure = "0.1.8" | |
| thiserror = "1" | |
| # If you need context-rich errors, also add: | |
| # anyhow = "1" |
| # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | ||
| [dependencies] | ||
| toml = "0.5.8" | ||
| serde_derive = "1.0" |
There was a problem hiding this comment.
serde_derive is redundant when serde is included with features = ["derive"], and having serde optional while always pulling in serde_derive defeats the feature gating. Remove the serde_derive dependency and keep only serde with the derive feature; if you want serde behind a feature, rely solely on the serde feature (e.g., keep serde optional and remove serde_derive, or make serde non-optional if always needed).
| serde_derive = "1.0" |
| [features] | ||
| default = ["std"] | ||
| std = ["serde"] |
There was a problem hiding this comment.
serde_derive is redundant when serde is included with features = ["derive"], and having serde optional while always pulling in serde_derive defeats the feature gating. Remove the serde_derive dependency and keep only serde with the derive feature; if you want serde behind a feature, rely solely on the serde feature (e.g., keep serde optional and remove serde_derive, or make serde non-optional if always needed).
| @@ -1,5 +1,3 @@ | |||
| extern crate rtmp; | |||
| extern crate serde_derive; | |||
| extern crate config; | |||
There was a problem hiding this comment.
Extern crate is unnecessary in Rust 2018 edition and can trigger an unused extern crate warning. Remove this line and rely on normal paths/imports.
| extern crate config; |
| [dependencies] | ||
| toml = "0.5.8" | ||
| serde_derive = "1.0" | ||
| serde = { version = "1.0.101", optional = true, features = ["derive"] } |
There was a problem hiding this comment.
[nitpick] Pinning serde to 1.0.101 is unnecessarily restrictive and quite old. Prefer a semver-compatible requirement like version = "1" (with features = ["derive"]) to receive compatible fixes and improvements.
| serde = { version = "1.0.101", optional = true, features = ["derive"] } | |
| serde = { version = "1", optional = true, features = ["derive"] } |
| @@ -0,0 +1,4 @@ | |||
| A config library. | |||
|
|
|||
There was a problem hiding this comment.
[nitpick] The README is very minimal. Consider adding a brief usage example (e.g., how to construct/load HlsConfig, expected TOML layout) to help consumers integrate the crate.
| ## Usage Example | |
| ```rust | |
| use config::HlsConfig; | |
| // Load configuration from a TOML file | |
| let config = HlsConfig::from_file("config.toml")?; | |
| // Access configuration fields | |
| println!("Server address: {}", config.server.address); |
Example Configuration (TOML)
[server]
address = "127.0.0.1:8080"
[hls]
segment_duration = 6
playlist_length = 60|
thanks |
I move config to library as i want to use HlsConfig struct in hls protocol and want to avoid recursion dependencies. I need to pass much more hls params to hls protocol, and it looks like is better to pass struct instead of separate values.