Allow log level selection #2739
Conversation
This comment has been minimized.
This comment has been minimized.
VictoriousRaptor
force-pushed
the
debug-log-level
branch
from
c3c13de to
e870dd2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Need some help on UI design.
|
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThe changes introduce new logging functionality within the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Settings
participant Logger
App->>Settings: Retrieve LogLevel value
alt LogLevel == "DEBUG"
App->>Logger: Call UseDebugLogLevel()
Logger-->>Logger: Log "Using DEBUG log level."
else LogLevel == "INFO"
App->>Logger: Call UseInfoLogLevel()
Logger-->>Logger: Log "Using INFO log level."
else Other
App->>Logger: Log error about unrecognized log level
App->>Logger: Call UseDebugLogLevel() as fallback
Logger-->>Logger: Log "Using DEBUG log level."
end
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (2)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
202-202: Consider using a property for LogLevel to maintain consistency.This new field adds the ability to configure log levels, but it's implemented as a public field rather than as a property with getters and setters like most other settings in this class.
-public string LogLevel = "info"; +public string LogLevel { get; set; } = "info";Additionally, consider adding XML documentation to explain the purpose of this setting and its valid values (e.g., "debug", "info").
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
66-74: Add log messages when changing log levels.The new methods provide a good API for switching log levels, but consider adding log messages to record when the log level is changed.
public static void UseDebugLogLevel() { LogManager.Configuration.FindRuleByName("file").SetLoggingLevels(LogLevel.Debug, LogLevel.Fatal); + Info("Log", "Changed log level to DEBUG"); } public static void UseInfoLogLevel() { LogManager.Configuration.FindRuleByName("file").SetLoggingLevels(LogLevel.Info, LogLevel.Fatal); + Info("Log", "Changed log level to INFO"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Infrastructure/Logger/Log.cs(1 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/App.xaml.cs(1 hunks)Flow.Launcher.Infrastructure/Logger/Log.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher/App.xaml.cs (1)
125-137: LGTM - Robust implementation for log level selection.The switch statement correctly handles the different log level settings including a default fallback to debug mode with appropriate error logging for unrecognized values.
One minor improvement suggestion: consider using just
nameof(App)for the error message on line 134 since the namespace is already included in the current context.Flow.Launcher.Infrastructure/Logger/Log.cs (1)
51-59: LGTM - Rule naming is necessary for the new level-switching functionality.The rule naming allows for finding and modifying specific rules in the configuration.
Docstrings generation was requested by @VictoriousRaptor. * #2739 (comment) The following files were modified: * `Flow.Launcher.Infrastructure/Logger/Log.cs` * `Flow.Launcher/App.xaml.cs`
|
Note Generated docstrings for this pull request at #3336 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Using If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
It works fine for me. Now you change log level in general settings page. |
|
🥷 Code experts: Jack251970, Yusyuriv Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
@VictoriousRaptor Check if this can work for you and feel free to merge it. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Flow.Launcher/Languages/en.xaml (1)
107-109: Good addition of log level localization strings.The new string resources for log level are well-placed in the general settings section of the localization file. Consider adding more log level options like "WARNING", "ERROR", and "FATAL" if those levels are supported in the logging system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Flow.Launcher.Infrastructure/Logger/Log.cs(2 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(2 hunks)Flow.Launcher/App.xaml.cs(1 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs(4 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Flow.Launcher/App.xaml.cs
- Flow.Launcher.Infrastructure/UserSettings/Settings.cs
- Flow.Launcher.Infrastructure/Logger/Log.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
🔇 Additional comments (3)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (3)
35-35: LGTM! Class definition for LogLevelData looks good.This follows the pattern of other settings dropdown data classes.
148-150: LGTM! LogLevels property initialization is consistent with other settings.The initialization of the LogLevels property follows the same pattern as other dropdown settings.
157-157: LGTM! Update to include LogLevels in localization updates.Properly updated the UpdateEnumDropdownLocalizations method to include the new LogLevels.
I find there is no need to restart app to take effect. And I think we can add this in general page. |
jjw24
added
enhancement
Closes #2317 #2313
Allow users to select log level. Set to "info" by default.