New API Function LogError #3408
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new LogError API function to support error logging for both C# and JSON RPC plugins.
- Added LogError method in PublicAPIInstance.cs that wraps Log.Error.
- Declared LogError in IPublicAPI and updated its XML documentation.
- Updated JsonRPCPublicAPI.cs to expose the new LogError method and made the _api field readonly.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Flow.Launcher/PublicAPIInstance.cs | Added LogError method to wrap Log.Error calls |
| Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs | Declared LogError in the public API interface along with XML documentation |
| Flow.Launcher.Infrastructure/Logger/Log.cs | Reordered using statements and removed the unused ExceptionInternal function |
| Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs | Added LogError implementation that delegates to the IPublicAPI and marked _api as readonly |
Comments suppressed due to low confidence (2)
Flow.Launcher/PublicAPIInstance.cs:190
- New LogError API introduced. Please add unit tests to validate its behavior in production scenarios.
public void LogError(string className, string message, [CallerMemberName] string methodName = "") =>
Flow.Launcher/Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs:158
- Consider adding unit tests for the new LogError method to ensure consistency and proper delegation to the underlying API.
public void LogError(string className, string message, [CallerMemberName] string methodName = "")
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: no user but you matched threshold 10 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: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
Jack251970
changed the title
📝 WalkthroughWalkthroughThe changes update the error logging functionality and code immutability across multiple components. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JSON_RPC as JsonRPCPublicAPI
participant API_Instance as PublicAPIInstance
participant Logger as Log
Client->>JSON_RPC: LogError(className, message, methodName)
JSON_RPC->>API_Instance: LogError(className, message, methodName)
API_Instance->>Logger: Log.Error(className, message, methodName)
Logger-->>API_Instance: Log processed
API_Instance-->>JSON_RPC: Log completed
JSON_RPC-->>Client: Log operation finished
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs(2 hunks)Flow.Launcher.Infrastructure/Logger/Log.cs(1 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
LogError(158-161)Flow.Launcher/PublicAPIInstance.cs (1)
LogError(190-191)
Flow.Launcher/PublicAPIInstance.cs (3)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
LogError(233-233)Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
LogError(158-161)Flow.Launcher.Infrastructure/Logger/Log.cs (4)
Log(13-229)Log(19-64)Error(178-181)Error(183-186)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
LogError(233-233)Flow.Launcher/PublicAPIInstance.cs (1)
LogError(190-191)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (4)
Flow.Launcher/PublicAPIInstance.cs (1)
190-192: LGTM: LogError method added to match interface implementationThis addition maintains consistency with the other logging methods in the class (LogDebug, LogInfo, LogWarn) and properly implements the interface contract from IPublicAPI.
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
230-234: LGTM: Clear interface contract for logging errorsThe added LogError method follows the same pattern as the existing logging methods with proper XML documentation. This provides developers with a cleaner way to log errors from plugins.
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (2)
15-15: Good improvement: Making _api readonlyMaking the _api field readonly is a good practice as it ensures immutability after initialization, improving thread safety.
158-161: LGTM: LogError method implements IPublicAPI contractThe implementation correctly delegates to the _api instance and maintains consistency with other logging methods in this class.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new LogError API function to support error logging for both C# and JSONRPC plugins while also refactoring some related formatting and field modifiers.
- Added LogError in PublicAPIInstance and JsonRPCPublicAPI.
- Updated IPublicAPI interface and removed an unused method in Logger/Log.
- Reformatted several methods for improved readability.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Flow.Launcher/PublicAPIInstance.cs | Introduces LogError API and reformats several method definitions. |
| Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs | Adds the LogError method definition to the public API interface. |
| Flow.Launcher.Infrastructure/Logger/Log.cs | Removes an unused ExceptionInternal method and adjusts usings. |
| Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs | Adds LogError method and marks the _api field as readonly. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
4-5: Using directive reorganizationThe reorganization of the using directives appears to be a minor stylistic change. The
System.Runtime.ExceptionServicesnamespace is needed for theExceptionDispatchInfo.Capture()calls on lines 108 and 170, and theUserSettingsnamespace is needed for accessingDataLocation.VersionLogDirectoryon line 21.While this reorganization doesn't affect functionality, maintaining a consistent ordering style (alphabetical or by namespace hierarchy) helps with code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/Logger/Log.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher.Infrastructure/Logger/Log.cs (2)
138-145:✅ Verification successful
The ExceptionInternal method implementation appears to be relocated rather than removed
The AI summary suggests this method was removed, but it appears to still be present within the
#if !DEBUGdirective. Since this method is called from lines 112 and 168, its presence is necessary. However, the previous review comment indicated that the method implementation was missing from the repository, suggesting a disconnect.Please verify that this method is properly implemented and accessible from all places it's being called, particularly in the non-DEBUG code paths.
🏁 Script executed:
#!/bin/bash # Verify ExceptionInternal method implementation and usage echo "Checking for ExceptionInternal method usage:" rg "ExceptionInternal\(" --type csharp echo -e "\nChecking if the ExceptionInternal method is properly implemented:" rg "private static void ExceptionInternal\(" --type csharpLength of output: 859
Clarification on ExceptionInternal Implementation
Our investigation confirms that the
ExceptionInternalmethod is indeed present and correctly implemented within the#if !DEBUGblock. Its usage in the file (at the call sites on lines 112 and 168) aligns with the intended logging functionality, which supports the PR objective for the LogError API. The previous AI summary that suggested the method was removed appears to be incorrect.
- Location:
Flow.Launcher.Infrastructure/Logger/Log.cs(lines 138-145 for the method implementation)- Action: No changes required for this method. Please ensure its behavior in non-DEBUG paths remains consistent with your logging design.
112-112:❓ Verification inconclusive
Cross-check LogError implementation with ExceptionInternal usage
According to the PR objectives, a new
LogErrorAPI function is being added. The existing error logging implementation in this file usesExceptionInternal, but this may be replaced or supplemented by the new API function in other files.Ensure that the changes to this file properly coordinate with the new
LogErrorimplementation in the C# and JSON-RPC plugins to maintain consistent error logging behavior across the application.Also applies to: 168-183
🏁 Script executed:
#!/bin/bash # Search for the new LogError implementation echo "Searching for new LogError implementations..." rg "LogError" --type csharp # Check the JsonRPC and C# plugin files for related changes echo -e "\nChecking JsonRPC files for related changes..." rg "JsonRPCPublicAPI" --type csharp -A 5 -B 5 | grep -i "log" echo -e "\nChecking PublicAPIInstance for LogError implementation..." rg "PublicAPIInstance" --type csharp -A 5 -B 5 | grep -i "log"Length of output: 1066
Action: Verify Consistency Between Legacy and New Error Logging Methods
- The file
Flow.Launcher.Infrastructure/Logger/Log.cs(lines 112 and 168–183) continues to useExceptionInternalfor error logging.- New
LogErrorimplementations are present in:
Flow.Launcher/PublicAPIInstance.csFlow.Launcher/Plugin/Interfaces/IPublicAPI.csFlow.Launcher/Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs- Please verify that the logic and behavior of the legacy
ExceptionInternalremain consistent with (or are appropriately coordinated to) the newLogErrorAPI across both C# and JSON-RPC plugins—ensuring uniform error reporting throughout the application.
| @@ -135,12 +135,14 @@ private static string CheckClassAndMessageAndReturnFullClassWithMethod(string cl | |||
| return className; | |||
| } | |||
|
|
|||
| #if !DEBUG | |||
There was a problem hiding this comment.
Why would we need DEBUG macro here? Is ExceptionInternal only used in DEBUG mode?
There was a problem hiding this comment.
Why would we need DEBUG macro here? Is
ExceptionInternalonly used in DEBUG mode?
yes, this function is only used in
There was a problem hiding this comment.
I think there's no need for this. It is ok for the debug version to contain extra symbol.
There was a problem hiding this comment.
If we alter the usage in log.cs we need to also tweak it here.
There was a problem hiding this comment.
If we alter the usage in log.cs we need to also tweak it here.
Could you please explain it further?
There was a problem hiding this comment.
If we alter the usage in log.cs we need to also tweak it here.
Could you please explain it further?
For example we adapt this change then delete the #if DEBUG else ... in the reference line you post, it will raise a compile error in DEBUG mode since compiler can't find this func.
There was a problem hiding this comment.
well. just keep as it was
| @@ -135,12 +135,14 @@ private static string CheckClassAndMessageAndReturnFullClassWithMethod(string cl | |||
| return className; | |||
| } | |||
|
|
|||
| #if !DEBUG | |||
There was a problem hiding this comment.
I think there's no need for this. It is ok for the debug version to contain extra symbol.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a LogError API function to both the C# and JSON-RPC plugins, enabling consistent error logging across different components.
- Introduces a new LogError method in PublicAPIInstance.cs and IPublicAPI.cs.
- Updates JsonRPCPublicAPI.cs to forward LogError calls to the underlying API.
- Performs minor formatting and using-directive reordering updates across affected files.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Flow.Launcher/PublicAPIInstance.cs | Adds LogError method and adjusts using directives, event formatting, and code style. |
| Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs | Adds the LogError function signature and XML comment for the new logging functionality. |
| Flow.Launcher.Infrastructure/Logger/Log.cs | Reorders using directives to include necessary namespaces. |
| Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs | Marks _api as readonly and introduces LogError forwarding to maintain API consistency. |
| @@ -156,6 +155,11 @@ public void LogWarn(string className, string message, [CallerMemberName] string | |||
| _api.LogWarn(className, message, methodName); | |||
| } | |||
|
|
|||
| public void LogError(string className, string message, [CallerMemberName] string methodName = "") | |||
There was a problem hiding this comment.
I'm not familiar with the [CallerMemberName] syntax. Is that just apart of the typing of the methodName parameter?
How would this work for json-rpc plugins? Would they make a request like (this is a mockup, I don't remember the exact structure):
{
"method": "LogError",
"params": ["plugin_name", "traceback/stacktrace", "name of method that the plugin was handling"]
}There was a problem hiding this comment.
I'm not familiar with the
[CallerMemberName]syntax. Is that just apart of the typing of themethodNameparameter?How would this work for json-rpc plugins? Would they make a request like (this is a mockup, I don't remember the exact structure):
{ "method": "LogError", "params": ["plugin_name", "traceback/stacktrace", "name of method that the plugin was handling"] }
No sure how to use it, I think we need to manually pass methodName. Just ignore [CallMemberName] which should be used by csharp plugins only.
And usage is the same as LogInfo etc.
Jack251970
changed the title
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. If the flagged items are 🤯 false positivesIf items relate to a ...
|
Add
LogErrorAPI function fromLog.Errorfor both csharp and jsonrpc pluginsFirstly, many preinstalled plugins use
Log.Errorfunction and I want to convert them into api usage.Secondly, for JsonRPC plugins, they do not have
LogExceptionand providing them with this api can give them ability to log exception-level information.