feat: add optional colored log output #52
Conversation
WalkthroughThe changes involve the addition of a new dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RunMain
participant RestApiClient
participant GithubApiClient
User->>RunMain: Start process
RunMain->>RestApiClient: start_log_group("Get list of specified source files")
RestApiClient->>GithubApiClient: start_log_group("Fetching files")
GithubApiClient-->>RestApiClient: Log files fetched
RestApiClient->>RunMain: end_log_group()
RunMain->>RestApiClient: start_log_group("Posting feedback")
RestApiClient->>GithubApiClient: post_feedback()
GithubApiClient-->>RestApiClient: Log feedback posted
RestApiClient->>RunMain: end_log_group()
RunMain-->>User: Process completed
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 (6)
cpp-linter/Cargo.toml (1)
20-20: LGTM! Consider using a version range forcolored.The addition of the
coloreddependency aligns well with the PR objective of introducing optional colored log output. This library is known for its cross-platform compatibility for terminal color display, which is excellent.Consider using a version range to allow for minor updates and bug fixes:
-colored = "2.1.0" +colored = "^2.1.0"This change would allow updates to patch and minor versions, potentially including important bug fixes, while still maintaining compatibility.
cpp-linter/src/rest_api/github/mod.rs (3)
98-101: LGTM! Consider adding a brief doc comment.The implementation of
start_log_grouplooks good. It correctly uses the GitHub Actions syntax for starting a log group.Consider adding a brief doc comment explaining that this method is specific to GitHub Actions log grouping. For example:
/// Starts a collapsible group in GitHub Actions logs. /// This is specific to GitHub Actions and may not work in other CI environments. fn start_log_group(&self, name: String) { // ... (existing implementation) }
103-106: LGTM! Consider adding a brief doc comment.The implementation of
end_log_grouplooks good. It correctly uses the GitHub Actions syntax for ending a log group.Consider adding a brief doc comment explaining that this method is specific to GitHub Actions log grouping. For example:
/// Ends a collapsible group in GitHub Actions logs. /// This is specific to GitHub Actions and may not work in other CI environments. fn end_log_group(&self) { // ... (existing implementation) }
98-106: Consider a more flexible approach for log grouping.While the current implementation correctly adds log grouping functionality for GitHub Actions, it might be worth considering a more flexible approach that could accommodate different CI environments in the future.
Here are a few suggestions to improve flexibility:
Consider creating an enum for different CI environments:
pub enum CIEnvironment { GitHub, GitLab, // Add other CI environments as needed }Modify the
RestApiClienttrait to include a method for getting the current CI environment:fn get_ci_environment(&self) -> CIEnvironment;Update the log grouping methods to use the CI environment:
fn start_log_group(&self, name: String) { match self.get_ci_environment() { CIEnvironment::GitHub => log::info!(target: "CI_LOG_GROUPING", "::group::{}", name), CIEnvironment::GitLab => log::info!(target: "CI_LOG_GROUPING", "section_start:`date +%s`:{}[collapsed=true]", name), // Add other CI environments as needed } } fn end_log_group(&self) { match self.get_ci_environment() { CIEnvironment::GitHub => log::info!(target: "CI_LOG_GROUPING", "::endgroup::"), CIEnvironment::GitLab => log::info!(target: "CI_LOG_GROUPING", "section_end:`date +%s`"), // Add other CI environments as needed } }This approach would make it easier to extend support for other CI environments in the future while maintaining the current functionality for GitHub Actions.
cpp-linter/src/logger.rs (2)
68-69: Reset environment variable after test to prevent side effects.The test sets the
CPP_LINTER_COLORenvironment variable but does not unset it afterward. This could inadvertently affect other tests or parts of the program that rely on this environment variable.Add the following at the end of the test to clean up:
// At the end of the test function env::remove_var("CPP_LINTER_COLOR");This ensures the environment remains clean for subsequent tests.
61-61: Rename test module to adhere to Rust conventions.The test module is currently named
test, but the conventional name istests. Renaming it improves code readability and aligns with standard Rust practices.-#[cfg(test)] -mod test { +#[cfg(test)] +mod tests {
📜 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 (7)
- README.md (2 hunks)
- cpp-linter/Cargo.toml (1 hunks)
- cpp-linter/src/clang_tools/mod.rs (3 hunks)
- cpp-linter/src/logger.rs (1 hunks)
- cpp-linter/src/rest_api/github/mod.rs (1 hunks)
- cpp-linter/src/rest_api/mod.rs (3 hunks)
- cpp-linter/src/run.rs (3 hunks)
🔇 Additional comments (15)
cpp-linter/Cargo.toml (2)
20-25: Summary: Changes align well with PR objectivesThe modifications to
Cargo.tomlare minimal and focused, aligning perfectly with the PR objective of adding optional colored log output. The addition of thecoloredcrate and the update to thelogcrate's features are appropriate for this purpose.These changes lay the groundwork for implementing the colored output feature. As development progresses, ensure that:
- The
CPP_LINTER_COLORenvironment variable is properly implemented and documented.- The colored output respects terminal capabilities and user preferences.
- The feature remains optional and doesn't interfere with existing functionality.
Great job on keeping the changes focused and relevant!
25-25: LGTM! Can you clarify the reason for adding the "std" feature?The update to the
logdependency, adding the "std" feature, looks good. This change suggests that standard library features of thelogcrate will be used.Could you please clarify the specific reason for adding the "std" feature? Is it related to the new colored output functionality, or are there other considerations?
To help understand the impact of this change, let's check for usage of
logcrate features that require "std":This will help us understand if these specific features are being used in the codebase.
✅ Verification successful
Standard library features for the
logcrate are necessaryThe usage of
log::set_boxed_loggerincpp-linter/src/logger.rsrequires the "std" feature. Therefore, addingfeatures = ["std"]to thelogdependency is justified.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of log crate features that require "std" # Search for use of log::set_boxed_logger rg --type rust 'log::set_boxed_logger' # Search for use of log::set_logger_racy rg --type rust 'log::set_logger_racy' # Search for use of log::set_max_level_racy rg --type rust 'log::set_max_level_racy'Length of output: 186
README.md (1)
213-213: LGTM: New dependency and license information added correctly.The changes in the README.md file accurately reflect the addition of the new 'colored' dependency as described in the PR objectives. The license information (MPL-2.0) for the 'colored' crate has been correctly added to the list of dependencies.
These additions are consistent with the PR's goal of introducing optional colored log output functionality.
To ensure the accuracy of the added information, let's verify the 'colored' crate's license:
Also applies to: 227-227
cpp-linter/src/rest_api/mod.rs (5)
52-53: LGTM: New methodstart_log_groupadded toRestApiClienttraitThe addition of the
start_log_groupmethod to theRestApiClienttrait is well-defined and aligns with the PR objective of introducing optional colored log output.
55-56: LGTM: New methodend_log_groupadded toRestApiClienttraitThe addition of the
end_log_groupmethod to theRestApiClienttrait is well-defined and complements thestart_log_groupmethod, aligning with the PR objective.
463-465: LGTM: Implementation ofstart_log_groupforTestClientThe implementation of
start_log_groupforTestClientis simple and appropriate for a test client. It correctly uses thelogcrate to output an info message with the group name.
467-469: LGTM: Implementation ofend_log_groupand updates todummy_coveragetestThe implementation of
end_log_groupforTestClientis consistent withstart_log_group. The updates to thedummy_coveragetest appropriately include calls to both new methods, ensuring test coverage for the new functionality.Also applies to: 476-476, 492-493
52-56: Overall assessment: Changes are well-implemented and align with PR objectivesThe additions to the
RestApiClienttrait and its implementation inTestClientsuccessfully introduce the log grouping functionality. The changes are consistent, well-integrated, and properly tested. These modifications align well with the PR objective of enhancing logging capabilities and relocating log grouping functions to therest_apimodule.Also applies to: 463-469, 476-476, 492-493
cpp-linter/src/logger.rs (1)
37-38: Handle color output when colors are disabled.When colors are disabled (e.g., in non-supporting terminals or when
set_override(false)), thelevel_colorfunction may still return color codes, leading to unreadable log output.To ensure
level_colorhandles disabled colors correctly, consider verifying the output without colors:// Temporarily disable colors set_override(false); let level_str = SimpleLogger::level_color(&Level::Info); assert_eq!(level_str, " INFO"); // Expect plain text without color codesThis verification ensures that log output remains clean and readable regardless of color settings.
cpp-linter/src/run.rs (6)
20-21: Imports updated to includeGithubApiClientandRestApiClientThe import statements have been correctly updated to include
GithubApiClientandRestApiClient, which are necessary for the integration of therest_apimodule in the logging functions.
96-96: Log grouping functions transitioned torest_api_clientThe call to
rest_api_client.start_log_group("Get list of specified source files")replaces the previous usage from theloggermodule, aligning with the objective to relocate log grouping functions to therest_apimodule. This centralizes logging operations related to API interactions.
127-127: Ending log group withrest_api_clientThe call to
rest_api_client.end_log_group()correctly ends the log group initiated earlier, ensuring proper logging structure.
131-137: Updatedcapture_clang_tools_outputto include&rest_api_clientThe function
capture_clang_tools_outputnow includes&rest_api_clientas an argument, indicating tighter integration with the API client during the feedback capturing process. Ensure that the function's definition and implementation have been updated accordingly to handle this new parameter.
138-138: Started log group for feedback postingThe addition of
rest_api_client.start_log_group("Posting feedback")enhances the logging granularity by grouping logs related to feedback posting, which improves readability and debugging.
142-142: Ended log group after feedback postingThe call to
rest_api_client.end_log_group()appropriately closes the log group for feedback posting, maintaining proper log structure.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
+ Coverage 97.45% 97.48% +0.03%
==========================================
Files 14 14
Lines 3295 3338 +43
==========================================
+ Hits 3211 3254 +43
Misses 84 84 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cpp-linter/src/logger.rs (2)
13-22: LGTM: Effective color formatting for log levels.The
level_colormethod provides a clear and intuitive color scheme for different log levels, enhancing readability. The implementation is concise and covers all log levels appropriately.Consider using
to_string()consistently for all match arms:Level::Error => name.red().bold().to_string(), Level::Warn => name.yellow().bold().to_string(), Level::Info => name.green().bold().to_string(), Level::Debug => name.blue().bold().to_string(), Level::Trace => name.magenta().bold().to_string(),This minor change would improve consistency across the match expression.
1-73: Approval: Removal of log grouping functions.The removal of
start_log_groupandend_log_groupfunctions aligns with the PR objectives. The log grouping functionality is now handled more appropriately within thelogmethod for CI-specific cases.Consider updating the module-level documentation to reflect these changes and explain how CI log grouping is now handled. This will help maintain clear and up-to-date documentation for users of this module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- cpp-linter/src/logger.rs (1 hunks)
🔇 Additional comments (5)
cpp-linter/src/logger.rs (5)
3-11: LGTM: New imports and logger structure.The new imports and the
SimpleLoggerstruct definition align well with the PR objectives. The use ofanyhowfor error handling andcoloredfor terminal output is appropriate. TheDefaultderive forSimpleLoggerprovides a clean implementation.
26-28: Great job addressing the previous feedback!The
enabledmethod now correctly respects the global maximum log level set bylog::max_level(). This change allows for more flexible control over logging levels at runtime and resolves the potential issue with ignored log messages.
30-40: LGTM: Improved log method with CI-specific handling.The
logmethod implementation effectively incorporates the new colored output while also handling CI-specific log grouping. The use oflevel_colorenhances the log output as intended, and the method correctly checks if the log should be enabled before processing.
50-57: Excellent improvements to theinitfunction!The changes to the
initfunction effectively implement the optional colored output feature while addressing previous compatibility concerns:
- The function now returns
Result<()>, allowing for better error handling.- The environment variable check uses
matches!, resolving the compatibility issue with older Rust versions.- The use of
log::set_boxed_loggerprovides a more flexible approach compared to the previous staticLOGGER.These changes align well with the PR objectives and improve the overall robustness of the logging initialization.
61-73: Great job on improving test coverage!The changes to the test module effectively address previous concerns and improve coverage:
- The new
trace_logtest verifies both the colored output functionality and trace-level logging.- Setting
CPP_LINTER_COLORenvironment variable ensures the color feature is tested.- The use of
log::set_max_level(log::LevelFilter::Trace)correctly enables trace-level logging for the test.These improvements ensure that the new features are properly tested and validate the logger's behavior at all log levels.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cpp-linter/src/logger.rs (1)
64-76: LGTM with a minor suggestion: Test coverage is improved.The new
trace_logtest effectively covers the colored output functionality and trace-level logging. It's great to see that you're setting the environment variable, initializing the logger, and verifying both the color output and trace-level logging.Consider adding an assertion to verify that the log message is actually output. You could capture the stdout and check its contents. Here's a suggestion:
use std::io::Cursor; #[test] fn trace_log() { env::set_var("CPP_LINTER_COLOR", "true"); init().unwrap_or(()); assert!(SimpleLogger::level_color(&log::Level::Trace).contains("TRACE")); log::set_max_level(log::LevelFilter::Trace); let mut buffer = Vec::new(); let _guard = log::set_logger(&SimpleLogger).map(|()| log::set_max_level(LevelFilter::Trace)); { let mut writer = Cursor::new(&mut buffer); log::set_boxed_logger(Box::new(SimpleLogger)).unwrap(); log::trace!("A dummy log statement for code coverage"); } let log_output = String::from_utf8(buffer).unwrap(); assert!(log_output.contains("A dummy log statement for code coverage")); }This addition would provide more comprehensive test coverage by ensuring that the log message is actually output as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- cpp-linter/src/logger.rs (1 hunks)
🧰 Additional context used
📓 Learnings (1)
cpp-linter/src/logger.rs (1)
Learnt from: 2bndy5 PR: cpp-linter/cpp-linter-rs#52 File: cpp-linter/src/logger.rs:13-22 Timestamp: 2024-10-02T08:00:06.515Z Learning: When a suggestion has already been made and acknowledged by the user, avoid repeating the same suggestion in subsequent comments.
🔇 Additional comments (5)
cpp-linter/src/logger.rs (5)
3-7: LGTM: Import statements are appropriate for the new features.The new imports (
anyhow,colored, and additionallogitems) align well with the PR objectives. The use ofanyhowfor error handling andstd::envfor environment variable checking are good practices.
9-23: LGTM: SimpleLogger struct and level_color method are well-implemented.The
SimpleLoggerstruct with itsDefaultderive is appropriately simple. Thelevel_colormethod effectively uses pattern matching to assign colors to log levels, following common color conventions. This implementation aligns well with the PR objective of adding optional colored log output.
25-42: LGTM: log::Log trait implementation is improved and aligns with PR objectives.The changes to the
log::Logtrait implementation forSimpleLoggerare well-done:
- The
enabledmethod now correctly respectslog::max_level(), addressing the previous suggestion.- The
logmethod appropriately handles CI-specific logging separately.- Colored output is implemented as intended, using the
level_colormethod.These changes effectively fulfill the PR objectives and improve the logging functionality.
50-60: LGTM: init function improvements enhance flexibility and error handling.The changes to the
initfunction are well-implemented:
- The return type
Result<()>allows for more flexible error handling withanyhow.- The
CPP_LINTER_COLORenvironment variable check correctly implements the optional colored output feature.- Using
log::set_boxed_loggerinstead of a static LOGGER provides more flexibility.- Error mapping to
anyhow::Errorimproves error handling consistency.These changes align well with the PR objectives and improve the overall design.
1-76: Great job implementing colored log output and improving logger functionality!The changes in this file effectively address the PR objectives and improve the overall logging functionality:
- Colored output is implemented using the
coloredcrate, with an environment variable toggle.- Error handling is enhanced with the use of
anyhow.- The logger initialization process is more flexible with
set_boxed_logger.- Test coverage has been expanded to include the new functionality.
The code is well-structured, follows Rust best practices, and there are no apparent issues or inconsistencies. These changes will significantly improve the user experience by providing optional colored log output and better error handling.
Uses a new dependency
coloredto get cross-platform compatible for terminal colors. Supports an environment variableCPP_LINTER_COLORthat (if set to1,on, ortrue) will force color output when it is automatically disabled (seecoloredfeatures which follows standards).I also moved the log grouping functions into the
rest_apimodule. Technically, the log grouping commands we currently use are specific to GitHub. Other Git servers (like GitLab) implement these differently.Summary by CodeRabbit
Release Notes
New Features
colored, enhancing log message visibility with color coding.start_log_groupandend_log_groupto improve log organization within the API client.Bug Fixes
GithubApiClientfor better feedback management.Documentation
README.mdto reflect the addition of the newcoloreddependency and its license information.Chores