feat!: provide sub command "server run" (#600) #602
Conversation
This moves "run-server" to "server run". Release-As: 0.30.0
WalkthroughThe pull request introduces multiple updates across various files, primarily focusing on enhancing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Server
participant API
User->>CLI: Execute command (Server)
CLI->>Server: Process command
Server->>API: Handle request
API->>Server: Return response
Server->>CLI: Send response to CLI
CLI->>User: Display response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (16)
utils/docker/entrypoint.sh (2)
Line range hint
33-33: Remove redundant exit statement.The
exit $?statement is unnecessary as theexeccommand replaces the current process, making this line unreachable.-exit $?
Line range hint
1-1: Consider using a more portable shebang.The current shebang assumes bash is installed at
/usr/bin/bash. For better portability across different Docker images, consider using/bin/bashor the env-based shebang.-#!/usr/bin/bash +#!/usr/bin/env bashbuild.rs (1)
36-38: LGTM! Well-implemented built integration with proper error handling.The error handling is implemented correctly using anyhow. However, consider making the error message slightly more specific.
Consider this minor improvement to the error message:
- built::write_built_file().map_err(|e| anyhow::anyhow!("Failed to write built file: {}", e))?; + built::write_built_file().map_err(|e| anyhow::anyhow!("Failed to write build-time information file: {}", e))?;src/server/schema.rs (2)
1-11: Enhance module documentation for better clarity.Consider expanding the module documentation to include:
- The purpose of schema dumping
- The output format (OpenAPI/YAML)
- Example usage
-//! Dump schema of the REST API server. +//! Module for dumping OpenAPI schema of the REST API server. +//! +//! This module provides functionality to generate and export the OpenAPI schema +//! in YAML format, either to a file or stdout. The schema describes all available +//! REST API endpoints and their specifications. +//! +//! # Example +//! +//! ```bash +//! mehari server schema --output-file api-schema.yaml +//! ```
12-29: Enhance error handling and field documentation.The implementation is good, but consider these improvements:
- Add more specific documentation for the output_file field
- Enhance error handling with custom error messages
#[derive(clap::Parser, Debug, Clone)] #[command(author, version, about = "Dump REST API schema", long_about = None)] pub struct Args { - /// Path to the output file. Use stdout if missing. + /// Path to write the OpenAPI schema YAML output. + /// If not specified, the schema will be written to stdout. + /// Example: "api-schema.yaml" #[arg(long)] pub output_file: Option<String>, } impl Args { /// Get writeable output file or stdout. fn get_output(&self) -> Result<Box<dyn Write>, io::Error> { match self.output_file { - Some(ref path) => File::create(path).map(|f| Box::new(f) as Box<dyn Write>), + Some(ref path) => File::create(path) + .map_err(|e| io::Error::new( + e.kind(), + format!("Failed to create output file '{}': {}", path, e) + )) + .map(|f| Box::new(f) as Box<dyn Write>), None => Ok(Box::new(io::stdout())), } } }src/server/run/actix_server/mod.rs (1)
Line range hint
17-32: Consider preserving error context while maintaining API documentation.While the current implementation supports serialization and API documentation, converting
anyhow::Errorto a String loses valuable error context and stack traces that could be useful for debugging.Consider this alternative implementation that preserves error details:
-#[derive(Debug, serde::Serialize, utoipa::ToSchema)] -pub struct CustomError { - err: String, -} +#[derive(Debug, serde::Serialize, utoipa::ToSchema)] +pub struct CustomError { + message: String, + #[serde(skip_serializing)] + #[schema(hidden = true)] + source: Option<anyhow::Error>, +} impl CustomError { fn new(err: anyhow::Error) -> Self { CustomError { - err: err.to_string(), + message: err.to_string(), + source: Some(err), } } }This way:
- API clients still get a clean error message
- Internal error handling preserves the full context
- The error source is excluded from API documentation
Cargo.toml (1)
86-88: Good addition of OpenAPI documentation support.The addition of
utoipaand related crates will provide automatic OpenAPI documentation generation and Swagger UI integration, which is excellent for API discoverability and documentation.Consider documenting the following in your README:
- The OpenAPI documentation endpoint
- How to access the Swagger UI
- Any authentication requirements for the documentation endpoints
src/common/io/std.rs (1)
Line range hint
21-31: Consider enhancing error documentation.While the function is well documented for its basic usage, it would be helpful to document the possible error cases that could occur (e.g., file not found, permission issues, corrupted gzip data). This would help API consumers better handle potential errors.
Example addition to the docs:
/// # Arguments /// /// * `path` - A path to the file to open. +/// +/// # Errors +/// +/// Returns an error if: +/// * The file does not exist +/// * The process lacks permissions to read the file +/// * The gzip data is corrupted (for .gz files)src/main.rs (1)
Line range hint
1-57: Update CLI documentation in file header.The file header documentation needs to be updated to reflect the new server commands. The current help output example doesn't show the new
servercommand and its subcommands.src/annotate/seqvars/provider.rs (2)
139-142: Track TODOs for version implementations.The TODO comments indicate that proper version implementations are needed. Consider creating GitHub issues to track the implementation of proper versioning mechanisms.
Would you like me to create GitHub issues to track:
- Implementation of proper data versioning
- Implementation of proper schema versioning
Line range hint
300-309: Improve version string construction.The current version string construction has several potential issues:
- Using Debug formatting (
{:?}) forassemblyandconfigmight produce unstable strings- Using empty strings as fallbacks for missing versions could mask configuration issues
Consider this alternative implementation:
- let data_version = format!( - "{:?}{}{}{:?}", - assembly, - tx_seq_db.version.as_ref().unwrap_or(&"".to_string()), - tx_seq_db.genome_release.as_ref().unwrap_or(&"".to_string()), - config - ); + let data_version = format!( + "assembly={};version={};release={};config={}", + assembly.to_string(), + tx_seq_db.version.as_deref().unwrap_or("unknown"), + tx_seq_db.genome_release.as_deref().unwrap_or("unknown"), + serde_json::to_string(&config).unwrap_or_else(|_| "invalid".to_string()) + );src/server/run/actix_server/versions.rs (3)
80-87: Populate Version Information inDataVersionEntryCurrently,
version_refseqandversion_ensemblare set toNoneinDataVersionEntry::from_provider. If theProvidercontains version information for RefSeq or Ensembl, consider populating these fields to provide more detailed data version info in the API response.You can modify the method to extract version information from the
Provider:pub fn from_provider(provider: &Provider) -> Self { let genome_build = Assembly::from(provider.assembly()); + let version_refseq = provider.refseq_version().map(|v| v.to_string()); + let version_ensembl = provider.ensembl_version().map(|v| v.to_string()); Self { genome_build, - version_refseq: None, - version_ensembl: None, + version_refseq, + version_ensembl, } }
131-131: Simplify Error Wrapping in Error MappingWhen mapping the error in the handler, you wrap the error with additional context. Since
CustomError::newalready accepts ananyhow::Error, you can pass the original error directly to avoid redundant wrapping.Apply this minor adjustment:
.map_err(|e| CustomError::new(e))?,
113-133: Enhance Documentation for the Endpoint HandlerThe
handlefunction lacks documentation comments explaining its purpose and usage. Adding Rust doc comments will improve code readability and maintainability, aiding other developers in understanding the endpoint's functionality.Consider adding documentation like:
/// Handler for the `/api/v1/versionsInfo` endpoint. /// /// Returns version information for the software and data. /// /// # Errors /// /// Returns a `CustomError` if fetching version information fails.src/server/run/mod.rs (2)
81-82: Fix typo in comment.There's a typo in the comment on line 81. The word "comput ethe" should be corrected to "compute the".
Apply this diff to fix the typo:
-// The endpoint `/tx/csq` to comput ethe consequence of a variant; without and with filtering +// The endpoint `/seqvars/csq` to compute the consequence of a variant; without and with filtering
103-103: Correct the endpoint name in the comment.The comment mentions
/structvars/csq, but the correct endpoint is/strucvars/csq.Apply this diff to fix the typo:
-// The endpoint `/structvars/csq` computes the consequence of an SV. +// The endpoint `/strucvars/csq` computes the consequence of an SV.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(3 hunks)build.rs(2 hunks)src/annotate/seqvars/provider.rs(2 hunks)src/common/io/std.rs(1 hunks)src/lib.rs(1 hunks)src/main.rs(3 hunks)src/server/mod.rs(1 hunks)src/server/run/actix_server/gene_txs.rs(1 hunks)src/server/run/actix_server/mod.rs(4 hunks)src/server/run/actix_server/versions.rs(1 hunks)src/server/run/mod.rs(1 hunks)src/server/schema.rs(1 hunks)utils/docker/entrypoint.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/server/mod.rs
🔇 Additional comments (18)
utils/docker/entrypoint.sh (1)
28-28: LGTM: Command structure change aligns with PR objectives.
The change from run-server to server run follows a logical command grouping pattern and matches the PR's goal of restructuring server commands.
src/lib.rs (1)
30-33: LGTM! Build information module looks good.
The implementation follows Rust best practices for embedding build-time information using the built crate.
Let's verify the build system integration:
✅ Verification successful
Build system integration is properly configured ✅
The build system is correctly set up with:
builtcrate integration inbuild.rsfor generating build information- Proper dependency declarations in
Cargo.tomlwith required features - Correct error handling for the build info generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify build.rs configuration for the built crate
# Test 1: Check if build.rs exists and contains built crate configuration
rg -l "built" build.rs
# Test 2: Verify Cargo.toml has the necessary built dependency
rg "built.*=.*\".*\"" Cargo.toml
Length of output: 227
Script:
#!/bin/bash
# Let's verify the build.rs configuration
cat build.rs
Length of output: 1499
build.rs (1)
7-7: LGTM! Clear documentation of build script functionality.
The comment accurately describes the integration of prost-build and pbjson-build, improving code maintainability.
src/server/schema.rs (1)
31-51: 🛠️ Refactor suggestion
Add terminal validation and improve error handling.
The implementation could benefit from these improvements:
- Validate if stdout is a terminal to prevent accidental output
- Add progress indication for large schemas
- More specific error handling
pub fn run(args_common: &crate::common::Args, args: &Args) -> Result<(), anyhow::Error> {
tracing::info!("args_common = {:?}", &args_common);
tracing::info!("args = {:?}", &args);
+ // Warn if writing to terminal
+ if args.output_file.is_none() && atty::is(atty::Stream::Stdout) {
+ tracing::warn!("Writing schema to terminal. Consider redirecting to a file.");
+ }
+
+ tracing::info!("Generating OpenAPI schema...");
let schema_yaml = ApiDoc::openapi()
.to_yaml()
.map_err(|e| anyhow::anyhow!("Failed to convert OpenAPI to YAML: {}", e))?;
+
+ tracing::info!("Writing schema to output...");
let mut output = args
.get_output()
.map_err(|e| anyhow::anyhow!("Failed to open output file: {}", e))?;
- write!(output, "{}", &schema_yaml)
- .map_err(|e| anyhow::anyhow!("Failed to write output: {}", e))?;
+
+ // Write with better error context
+ write!(output, "{}", &schema_yaml).map_err(|e| {
+ let context = match &args.output_file {
+ Some(path) => format!("to file '{}'", path),
+ None => "to stdout".to_string(),
+ };
+ anyhow::anyhow!("Failed to write schema {}: {}", context, e)
+ })?;
tracing::info!("All done. Have a nice day!");
Ok(())
}src/server/run/actix_server/mod.rs (2)
6-6: LGTM: OpenAPI integration and versions module.
The addition of OpenAPI support and the new versions module are well-structured and align with the server restructuring objective.
Also applies to: 15-15
64-68: Verify OpenAPI documentation endpoint.
The Swagger UI integration looks correct, but let's verify the OpenAPI documentation endpoint is properly configured.
✅ Verification successful
OpenAPI documentation endpoint is properly configured
The verification confirms that:
- The OpenAPI documentation is correctly implemented with
ApiDocstruct inrun/openapimodule - The Swagger UI is properly configured at
/swagger-uiwith documentation at/api-docs/openapi.json - The API schema includes:
- Versioning endpoint with proper OpenAPI annotations
- Required component schemas (Assembly, CustomError, VersionsInfoResponse, etc.)
- Schema dumping functionality for documentation generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify OpenAPI documentation setup
# Test: Check if ApiDoc is properly implemented
ast-grep --pattern 'impl OpenApi for ApiDoc {
$$$
}'
# Test: Check for any potential duplicate Swagger UI configurations
rg -A 2 'SwaggerUi::new'
Length of output: 359
Script:
#!/bin/bash
# Let's check the ApiDoc struct definition and its implementation
ast-grep --pattern 'struct ApiDoc {
$$$
}'
# Check for OpenAPI schema definitions and routes
rg -l "utoipa::OpenApi|utoipa::Path"
# Check if there are any route handlers with OpenAPI documentation
ast-grep --pattern '#[utoipa::path($$$)]'
Length of output: 817
Script:
#!/bin/bash
# Let's check the OpenAPI schema implementation in schema.rs
cat src/server/schema.rs
# Check the ApiDoc implementation in run/mod.rs
cat src/server/run/mod.rs
# Check if there are any OpenAPI components or schemas defined
ast-grep --pattern '#[derive(Schema)]'
Length of output: 7190
src/server/run/actix_server/gene_txs.rs (1)
23-23: LGTM! Improved type safety with explicit return type.
The change from impl Responder to actix_web::Result<Json<GeneTranscriptsResponse>, super::CustomError> makes the API contract more explicit and provides better type safety. This is a positive change that aligns with Rust best practices for API design.
Cargo.toml (2)
36-36: LGTM: Build information integration looks good.
The addition of built with appropriate features in both runtime and build dependencies will provide useful build-time information.
Let's verify the build system integration:
Also applies to: 96-96
10-10: Verify the Rust version requirement bump.
The Rust version requirement has been increased from 1.70.0 to 1.75.0. This is a significant version bump that might affect users.
Let's verify if this update is necessary:
src/common/io/std.rs (1)
119-119: LGTM! Code simplification looks good.
The change from &format! to format! is a good simplification as it removes an unnecessary reference while maintaining the same functionality. The String returned by format! already implements AsRef<Path> which is what the function requires.
src/main.rs (4)
107-108: LGTM: Server command variant follows established patterns.
The addition of the Server variant to the Commands enum is well-structured and consistent with the existing command organization.
148-155: LGTM: Server command structure follows established patterns.
The Server struct implementation is consistent with other command structures in the codebase, using appropriate clap derive macros and documentation.
221-224: LGTM: Server command handling is well-implemented.
The match arm for Server commands follows the established pattern and properly handles both subcommands with appropriate error handling and async/await usage.
157-162: Verify the purpose of the Schema subcommand.
While the Run subcommand aligns with the PR objective of relocating "run-server", the Schema subcommand appears to be a new addition. Please ensure this addition is intentional and documented.
✅ Verification successful
Let me gather more information about the Schema subcommand.
The Schema subcommand is a well-documented addition for OpenAPI schema generation
The Schema subcommand is an intentional and documented addition that dumps the REST API schema in OpenAPI/YAML format. The implementation in src/server/schema.rs shows it's a utility command that:
- Generates OpenAPI documentation from the server's API endpoints
- Outputs the schema to either a file or stdout
- Includes proper command-line documentation and error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for documentation or related changes for the Schema subcommand
# Test: Look for documentation or related changes
rg -A 5 "server::schema"
rg -A 5 "Schema command"
Length of output: 609
Script:
#!/bin/bash
# Look for the schema module and its implementation
fd schema src/server/
cat src/server/schema.rs
# Look for any documentation or comments about schema in the codebase
rg -i "schema" README.md
rg -B 5 "Schema" src/ --type rust
Length of output: 4648
src/annotate/seqvars/provider.rs (2)
362-370: LGTM!
The assembly() method is well-documented and correctly implemented.
Line range hint 479-489: Note: Temporary version implementations.
The current implementations of data_version() and schema_version() are placeholders. This aligns with the TODO comments in the struct definition. Ensure these are updated when implementing proper versioning.
✅ Verification successful
The review comment appears to be discussing version implementations at lines 479-489, but the provided code snippet shows lines 136-144 which are about sequence handling. Let me verify the actual implementation of these version-related methods.
Let me try a different approach to locate these version-related implementations.
Verified: Version implementations are indeed temporary placeholders
The code shows that both data_version() and schema_version() methods are currently returning temporary values that are constructed during initialization to serve as cache keys. This matches exactly with what the review comment highlighted.
- The
Provider::new()constructs temporary versions using a format string that combines multiple values to ensure uniqueness - Both
data_version()andschema_version()implementations simply return references to these temporary values - TODO comments in both the constructor and implementations confirm this is a temporary solution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other version-related TODOs in the codebase
rg -i "todo.*version" --type rust
Length of output: 376
Script:
#!/bin/bash
# Get the full context of the version-related implementations
ast-grep --pattern 'fn data_version(&self) -> &str { $$ }'
ast-grep --pattern 'fn schema_version(&self) -> &str { $$ }'
Length of output: 123
Script:
#!/bin/bash
# Get more context around the TODO comments we found earlier
rg -B 5 -A 5 "TODO.*version" src/annotate/seqvars/provider.rs
Length of output: 873
src/server/run/actix_server/versions.rs (1)
34-42: Clarify Handling of Grch37p10 Assembly
The From implementation maps both Grch37 and Grch37p10 from biocommons_bioutils::assemblies::Assembly to Assembly::Grch37. Consider whether Grch37p10 should be distinguished from Grch37 in your application logic. If they require different handling, adding a distinct variant for Grch37p10 might be necessary.
To verify if Grch37p10 requires separate handling, you can search the codebase for its usage:
✅ Verification successful
Let me gather more context about the Assembly enum definition and its usage.
Based on the search results, I can now generate the final response.
Grch37p10 Mapping is Correct and Intentional
The current mapping is correct. The codebase explicitly documents and handles the distinction between Grch37 and Grch37p10: Grch37p10 is specifically used when chrMT (mitochondrial chromosome) support is needed, as noted in the comment "NB: Assembly::Grch37 does not contain chrMT, but Assembly::Grch37p10 does." The codebase consistently maps both to Grch37 for string representations while maintaining the distinction where chrMT handling is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of 'Grch37p10' in the codebase.
# Test: Search for 'Grch37p10' to determine if distinct handling is needed.
rg 'Assembly::Grch37p10'
Length of output: 2356
Script:
#!/bin/bash
# Find the Assembly enum definition
ast-grep --pattern 'enum Assembly {
$$$
}'
# Check for any comments or documentation about Assembly and Grch37p10
rg -B 2 -A 2 'Assembly::Grch37p10' src/annotate/strucvars/mod.rs
Length of output: 1703
src/server/run/mod.rs (1)
103-104:
Remove extra '&' in the sample endpoint URL.
Similar to a previous comment, there's an extra & in start=48275360&&stop=48275370. It should be start=48275360&stop=48275370.
Apply this diff to correct the URL:
-"&chromosome=17&start=48275360&&stop=48275370&sv_type=DEL",
+"&chromosome=17&start=48275360&stop=48275370&sv_type=DEL",| pub fn new() -> Result<Self, anyhow::Error> { | ||
| let mehari = crate::built_info::PKG_VERSION.to_string(); | ||
| let hgvs_rs = crate::built_info::DEPENDENCIES | ||
| .iter() | ||
| .find(|(name, _)| name == &"hgvs") | ||
| .map(|(_, version)| version.to_string()) | ||
| .ok_or_else(|| anyhow::anyhow!("Failed to find hgvs version"))?; | ||
|
|
||
| Ok(Self { mehari, hgvs_rs }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Missing hgvs Dependency Gracefully
In SoftwareVersions::new, if the hgvs crate is not found in crate::built_info::DEPENDENCIES, the function returns an error, which may cause the /api/v1/versionsInfo endpoint to fail. Consider providing a default value or marking the version as unknown to ensure the endpoint remains operational even if the hgvs version is unavailable.
Apply the following change to handle the missing dependency gracefully:
.map(|(_, version)| version.to_string())
-.ok_or_else(|| anyhow::anyhow!("Failed to find hgvs version"))?;
+.unwrap_or_else(|| "unknown".to_string());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn new() -> Result<Self, anyhow::Error> { | |
| let mehari = crate::built_info::PKG_VERSION.to_string(); | |
| let hgvs_rs = crate::built_info::DEPENDENCIES | |
| .iter() | |
| .find(|(name, _)| name == &"hgvs") | |
| .map(|(_, version)| version.to_string()) | |
| .ok_or_else(|| anyhow::anyhow!("Failed to find hgvs version"))?; | |
| Ok(Self { mehari, hgvs_rs }) | |
| } | |
| pub fn new() -> Result<Self, anyhow::Error> { | |
| let mehari = crate::built_info::PKG_VERSION.to_string(); | |
| let hgvs_rs = crate::built_info::DEPENDENCIES | |
| .iter() | |
| .find(|(name, _)| name == &"hgvs") | |
| .map(|(_, version)| version.to_string()) | |
| .unwrap_or_else(|| "unknown".to_string()); | |
| Ok(Self { mehari, hgvs_rs }) | |
| } |
| // The endpoint `/strucvars/csq` computes the consequence of an SV. | ||
| tracing::info!( | ||
| " try: http://{}:{}/strucvars/csq?genome_release=grch37\ | ||
| &chromosome=17&start=48275360&&stop=48275370&sv_type=DEL", |
There was a problem hiding this comment.
Remove extra '&' in the sample endpoint URL.
In the sample URL, there's an extra & in start=48275360&&stop=48275370. It should be start=48275360&stop=48275370.
Apply this diff to correct the URL:
-"&chromosome=17&start=48275360&&stop=48275370&sv_type=DEL",
+"&chromosome=17&start=48275360&stop=48275370&sv_type=DEL",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &chromosome=17&start=48275360&&stop=48275370&sv_type=DEL", | |
| &chromosome=17&start=48275360&stop=48275370&sv_type=DEL", |
| " try: http://{}:{}/genes/txs?hgncId=HGNC:1100&\ | ||
| genomeBuild=GENOME_BUILD_GRCH37", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent usage of genome release parameters in sample URLs.
In the sample endpoint URL, the parameter is genomeBuild=GENOME_BUILD_GRCH37, whereas in other endpoints, genome_release=grch37 is used. To maintain consistency and avoid confusion, consider using genome_release=grch37 here as well.
Apply this diff to correct the parameter:
-"&genomeBuild=GENOME_BUILD_GRCH37",
+"&genome_release=grch37",Committable suggestion skipped: line range outside the PR's diff.
| if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() { | ||
| std::env::set_var("RUST_LOG", "debug"); | ||
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid re-initializing the logger.
Calling env_logger::init_from_env inside the run function may cause a panic if the logger has already been initialized elsewhere in the application. To prevent this, consider checking if the logger is already initialized or initialize it only once at the program's entry point.
You can modify the logging setup to use env_logger::try_init:
- env_logger::init_from_env(env_logger::Env::new().default_filter_or("info"));
+ let _ = env_logger::try_init_from_env(env_logger::Env::new().default_filter_or("info"));This change will prevent a panic if the logger has been initialized previously.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() { | |
| std::env::set_var("RUST_LOG", "debug"); | |
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); | |
| } | |
| if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() { | |
| std::env::set_var("RUST_LOG", "debug"); | |
| let _ = env_logger::try_init_from_env(env_logger::Env::new().default_filter_or("info")); | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #602 +/- ##
==========================================
- Coverage 74.01% 73.38% -0.64%
==========================================
Files 26 28 +2
Lines 9863 9948 +85
==========================================
Hits 7300 7300
- Misses 2563 2648 +85 ☔ View full report in Codecov by Sentry. |
This moves "run-server" to "server run".
Release-As: 0.30.0
Summary by CodeRabbit
Release Notes
New Features
Servercommand for improved command-line interface organization.Improvements
Bug Fixes
Documentation