Conversation
Fix #1 Also update MostriX logo
Contributor
|
Caution Review failedThe pull request is closed. ## Walkthrough
This change introduces a configuration management system to the project. It adds the `config` and `serde` crates as dependencies, creates a `settings.toml` file with three settings (`mostro_pubkey`, `relays`, and `log_level`), and implements logic to load and manage these settings at runtime. A new `Settings` struct is defined and populated from the configuration file using deserialization. The initialization ensures that the configuration file exists in the user's home directory, copying a default if necessary, and makes the settings globally accessible within the application.
## Changes
| File(s) | Change Summary |
|------------------------|-----------------------------------------------------------------------------------------------------|
| Cargo.toml | Added `config` and `serde` dependencies with relevant features. |
| settings.toml | Introduced with fields: `mostro_pubkey`, `relays`, and `log_level`. |
| src/main.rs | Added logic to initialize, load, and print settings using the new configuration system; modified logger setup to accept log level string. |
| src/settings.rs | New module defining `Settings` struct and `init_settings()` for configuration management, including directory and default file setup. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Main
participant Settings
participant FileSystem
User->>Main: Start application
Main->>Settings: Call init_settings()
Settings->>FileSystem: Check/create config directory and settings.toml
FileSystem-->>Settings: Ensure settings.toml exists
Settings->>FileSystem: Read settings.toml
FileSystem-->>Settings: Return config data
Settings->>Settings: Deserialize config into Settings struct
Settings-->>Main: Return Settings reference
Main->>Main: Use settings (log, print, etc.)Assessment against linked issues
Possibly related PRs
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
src/main.rs (2)
302-306:⚠️ Potential issueUse relays from settings
The code currently hardcodes a single relay URL instead of using the relays defined in the settings.
// Configure Nostr client. let my_keys = Keys::generate(); let client = Client::new(my_keys); // Add the Mostro relay. - client.add_relay("wss://relay.mostro.network").await?; + // Add all relays from settings + for relay in &settings.relays { + client.add_relay(relay).await?; + } client.connect().await;
309-311:⚠️ Potential issueUse mostro_pubkey from settings
The Mostro public key is hardcoded instead of using the value from settings.
// Hardcoded Mostro Pubkey. - let mostro_pubkey_str = "82fa8cb978b43c79b2156585bac2c011176a21d2aead6d9f7c575c005be88390"; + let mostro_pubkey_str = &settings.mostro_pubkey; let mostro_pubkey = PublicKey::from_str(mostro_pubkey_str) .map_err(|e| anyhow::anyhow!("Invalid Mostro pubkey: {}", e))?;🧰 Tools
🪛 Gitleaks (8.21.2)
309-309: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🧹 Nitpick comments (1)
src/settings.rs (1)
16-18: Document purpose of the static variableThe comment is in Spanish. Consider translating it to English for consistency and to make it more broadly understandable.
-// Variable global, solamente se inicializa una vez +// Global variable that is initialized only once static SETTINGS: OnceLock<Settings> = OnceLock::new();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockstatic/logo.pngis excluded by!**/*.png
📒 Files selected for processing (4)
Cargo.toml(1 hunks)settings.toml(1 hunks)src/main.rs(3 hunks)src/settings.rs(1 hunks)
🔇 Additional comments (2)
Cargo.toml (1)
23-24: Dependencies look goodThe addition of the
configandserdecrates with appropriate features is a good choice for implementing the configuration management system.src/settings.rs (1)
1-8: Good module organizationThe imports are well-organized and the module structure is clean. Using
once_cell::sync::OnceLockis a good choice for lazy initialization of a global value.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/settings.rs (4)
22-22: Translate the Spanish example commentFor consistency, translate the Spanish example comment "p.e." to English.
- let package_name = env!("CARGO_PKG_NAME"); // p.e. "my_project" + let package_name = env!("CARGO_PKG_NAME"); // e.g. "my_project"
18-51: Improve error handling with Result instead of expectThe current implementation uses
expect()which terminates the program if an error occurs. Consider returning aResulttype for more graceful error handling, especially for a configuration function that might be called during startup.Here's how you could refactor the function:
-pub fn init_settings() -> &'static Settings { +pub fn init_settings() -> Result<&'static Settings, String> { SETTINGS.get_or_init(|| { - let home_dir = dirs::home_dir().expect("Could not find home directory"); + let home_dir = dirs::home_dir().ok_or("Could not find home directory")?; let package_name = env!("CARGO_PKG_NAME"); // e.g. "my_project" let hidden_dir = home_dir.join(format!(".{package_name}")); let hidden_file = hidden_dir.join("settings.toml"); // Path to the settings.toml included in the repo (next to Cargo.toml) let default_file: PathBuf = Path::new(env!("CARGO_MANIFEST_DIR")).join("settings.toml"); // Create ~/.mostrix if it doesn't exist if !hidden_dir.exists() { - fs::create_dir(&hidden_dir) - .expect("The configuration directory could not be created"); + fs::create_dir(&hidden_dir) + .map_err(|e| format!("The configuration directory could not be created: {}", e))?; } // Copy settings.toml if it isn't already in ~/.mostrix if !hidden_file.exists() { - fs::copy(&default_file, &hidden_file) - .expect("Could not copy default settings.toml"); + fs::copy(&default_file, &hidden_file) + .map_err(|e| format!("Could not copy default settings.toml: {}", e))?; } // Use the `config` crate to deserialize to the Settings struct let cfg = config::Config::builder() .add_source(config::File::from(hidden_file)) .build() - .expect("settings.toml malformed"); + .map_err(|e| format!("settings.toml malformed: {}", e))?; - cfg.try_deserialize::<Settings>() - .expect("Error deserializing settings.toml") + cfg.try_deserialize::<Settings>() + .map_err(|e| format!("Error deserializing settings.toml: {}", e)) }) + .map_err(|e| e) }This approach would allow the caller to handle configuration errors more gracefully, possibly by logging them and using default values, rather than terminating the program.
10-15: Consider adding validation for settingsThe
Settingsstruct currently just deserializes the values without any validation. Consider adding validation for fields likelog_levelto ensure they contain valid values.You could implement this by adding a
validatemethod to theSettingsstruct:impl Settings { /// Validates the settings values pub fn validate(&self) -> Result<(), String> { // Validate log_level match self.log_level.to_lowercase().as_str() { "trace" | "debug" | "info" | "warn" | "error" => Ok(()), _ => Err(format!("Invalid log_level: {}", self.log_level)) } // Add more validation for other fields as needed } }Then call this from the
init_settingsfunction after deserializing.
18-51: Use constants for fixed valuesThe code uses string literals like "settings.toml" in multiple places. Consider defining these as constants at the top of the file to improve maintainability.
use serde::Deserialize; use std::{ env, fs, path::{Path, PathBuf}, }; use dirs; +const SETTINGS_FILENAME: &str = "settings.toml"; #[derive(Debug, Deserialize)] pub struct Settings { // ... } pub fn init_settings() -> &'static Settings { SETTINGS.get_or_init(|| { // ... - let hidden_file = hidden_dir.join("settings.toml"); + let hidden_file = hidden_dir.join(SETTINGS_FILENAME); // Path to the settings.toml included in the repo (next to Cargo.toml) let default_file: PathBuf = - Path::new(env!("CARGO_MANIFEST_DIR")).join("settings.toml"); + Path::new(env!("CARGO_MANIFEST_DIR")).join(SETTINGS_FILENAME); // ... }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main.rs(4 hunks)src/settings.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.rs
🔇 Additional comments (1)
src/settings.rs (1)
1-8: 🛠️ Refactor suggestionAdd missing import for the dirs crate
The code uses
dirs::home_dir()on line 21, but there's no import for thedirscrate.use crate::SETTINGS; use serde::Deserialize; use std::{ env, fs, path::{Path, PathBuf}, }; +use dirs;Likely an incorrect or invalid review comment.
Merged
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #1
Also update MostriX logo
Summary by CodeRabbit