feat(SocketLogging): add SocketLogging function#556
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds SocketLogging functionality to the BootstrapBlazor.Socket library and performs namespace refactoring to improve code organization. The logging system provides centralized error tracking for socket operations.
- Introduces a new SocketLogging class for centralized logging in socket operations
- Refactors all namespaces from global BootstrapBlazor.* to BootstrapBlazor.Socket.* prefixes
- Updates project references and configurations to support the new namespace structure
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| DefaultTcpSocketFactory.cs | Adds SocketLogging initialization logic in the factory |
| SocketLogging.cs | New logging utility class with initialization and error logging methods |
| Multiple PropertyConverter files | Namespace changes from BootstrapBlazor.DataConverters to BootstrapBlazor.Socket.DataConverters |
| Multiple DataHandler files | Namespace changes from BootstrapBlazor.DataHandlers to BootstrapBlazor.Socket.DataHandlers |
| DataConverter.cs | Adds exception logging using the new SocketLogging class |
| BinConverter.cs | Namespace change with a typo in the new namespace |
| Project files | Version updates and dependency configuration changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| using System.Text; | ||
|
|
||
| namespace BootstrapBlazor.Components.DataConverter; | ||
| namespace BootstrapBlazorSocket.DataConverter; |
There was a problem hiding this comment.
The namespace has a typo - missing dot between 'BootstrapBlazor' and 'Socket'. Should be 'BootstrapBlazor.Socket.DataConverter' to match the pattern used in other files.
Suggested change
| namespace BootstrapBlazorSocket.DataConverter; | |
| namespace BootstrapBlazor.Socket.DataConverter; |
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(TargetFramework)' == 'net8.0'"> | ||
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(NET9Version)" /> |
There was a problem hiding this comment.
The .NET 8.0 target framework is incorrectly using NET9Version. Line 26 should use $(NET8Version) to match the pattern established for other .NET 8.0 dependencies.
Suggested change
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(NET9Version)" /> | |
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(NET8Version)" /> |
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(TargetFramework)' == 'net9.0'"> | ||
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(NET9Version)" /> |
There was a problem hiding this comment.
The .NET 8.0 target framework is incorrectly using NET9Version. Line 26 should use $(NET8Version) to match the pattern established for other .NET 8.0 dependencies.
Comment on lines
+33
to
+34
| <ItemGroup> | ||
| </ItemGroup> |
There was a problem hiding this comment.
Empty ItemGroup should be removed as it serves no purpose and clutters the project file.
Suggested change
| <ItemGroup> | |
| </ItemGroup> |
Comment on lines
+33
to
+35
| /// | ||
| /// </summary> | ||
| /// <param name="message"></param> |
There was a problem hiding this comment.
Empty XML documentation comment should be completed to describe the LogError method's purpose and parameters.
Suggested change
| /// | |
| /// </summary> | |
| /// <param name="message"></param> | |
| /// Logs an error message using the configured logger. | |
| /// </summary> | |
| /// <param name="message">The error message to log.</param> |
Comment on lines
+33
to
+42
| /// | ||
| /// </summary> | ||
| /// <param name="message"></param> | ||
| public static void LogError(string message) => _logger?.LogError(message); | ||
|
|
||
| /// <summary> | ||
| /// | ||
| /// </summary> | ||
| /// <param name="ex"></param> | ||
| /// <param name="message"></param> |
There was a problem hiding this comment.
Empty XML documentation comment should be completed to describe the LogError overload method's purpose and parameters.
Suggested change
| /// | |
| /// </summary> | |
| /// <param name="message"></param> | |
| public static void LogError(string message) => _logger?.LogError(message); | |
| /// <summary> | |
| /// | |
| /// </summary> | |
| /// <param name="ex"></param> | |
| /// <param name="message"></param> | |
| /// Logs an error message using the configured logger. | |
| /// </summary> | |
| /// <param name="message">The error message to log.</param> | |
| public static void LogError(string message) => _logger?.LogError(message); | |
| /// <summary> | |
| /// Logs an exception and an optional error message using the configured logger. | |
| /// </summary> | |
| /// <param name="ex">The exception to log.</param> | |
| /// <param name="message">An optional error message to log with the exception.</param> |
|
🧙 Sourcery is reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider replacing the static SocketLogging.Init call in GetOrCreate with proper ILogger injection (e.g. constructor injection) to avoid global mutable state and improve testability.
- Ensure the SocketLogging.Init path is thread-safe (for example by using a lock or Lazy initialization) to avoid race conditions when multiple sockets are created concurrently.
- Double-check the bulk namespace refactoring for consistency and typos (e.g. missing a '.' in some BootstrapBlazorSocket.DataConverter declarations) to prevent compilation errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing the static SocketLogging.Init call in GetOrCreate with proper ILogger injection (e.g. constructor injection) to avoid global mutable state and improve testability.
- Ensure the SocketLogging.Init path is thread-safe (for example by using a lock or Lazy initialization) to avoid race conditions when multiple sockets are created concurrently.
- Double-check the bulk namespace refactoring for consistency and typos (e.g. missing a '.' in some BootstrapBlazorSocket.DataConverter declarations) to prevent compilation errors.
## Individual Comments
### Comment 1
<location> `src/extensions/BootstrapBlazor.Socket/DataConverter/BinConverter.cs:7` </location>
<code_context>
using System.Text;
-namespace BootstrapBlazor.Components.DataConverter;
+namespace BootstrapBlazorSocket.DataConverter;
/// <summary>
</code_context>
<issue_to_address>
Namespace change introduces inconsistency with other files.
Please update the namespace to 'BootstrapBlazor.Socket.DataConverters' for consistency with the rest of the codebase and to prevent potential type resolution issues.
</issue_to_address>
### Comment 2
<location> `src/extensions/BootstrapBlazor.Socket/DataConverter/HexConverter.cs:5` </location>
<code_context>
using System.Text;
-namespace BootstrapBlazor.Components.DataConverter;
+namespace BootstrapBlazorSocket.DataConverter;
</code_context>
<issue_to_address>
Namespace 'BootstrapBlazor.Socket.DataConverter' is singular, while others use plural.
Consider renaming the namespace to 'BootstrapBlazor.Socket.DataConverters' to match the convention used elsewhere in the codebase.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
namespace BootstrapBlazor.Socket.DataConverter;
=======
namespace BootstrapBlazor.Socket.DataConverters;
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/extensions/BootstrapBlazor.TcpSocket/DefaultTcpSocketFactory.cs:20` </location>
<code_context>
public ITcpSocketClient GetOrCreate(string name, Action<TcpSocketClientOptions> valueFactory)
{
+ if (!SocketLogging.Inited)
+ {
+ var logger = provider.GetService<ILogger<DefaultTcpSocketFactory>>();
</code_context>
<issue_to_address>
SocketLogging initialization is performed per GetOrCreate call.
This approach can cause redundant initialization and race conditions under concurrent access. Recommend initializing SocketLogging once at startup or ensuring thread safety.
Suggested implementation:
```csharp
public DefaultTcpSocketFactory(IServiceProvider provider)
{
this.provider = provider;
// Ensure SocketLogging is initialized only once at startup
if (!SocketLogging.Inited)
{
var logger = provider.GetService<ILogger<DefaultTcpSocketFactory>>();
if (logger != null)
{
SocketLogging.Init(logger);
}
}
}
public ITcpSocketClient GetOrCreate(string name, Action<TcpSocketClientOptions> valueFactory)
{
return _pool.GetOrAdd(name, key =>
{
var options = new TcpSocketClientOptions();
```
- You may need to ensure that the `provider` field is defined at the class level and properly assigned in the constructor.
- If there are multiple constructors, make sure SocketLogging initialization is performed in all relevant ones.
- Remove any other redundant SocketLogging initialization from elsewhere in the class.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| using System.Text; | ||
|
|
||
| namespace BootstrapBlazor.Components.DataConverter; |
There was a problem hiding this comment.
issue: Namespace change introduces inconsistency with other files.
Please update the namespace to 'BootstrapBlazor.Socket.DataConverters' for consistency with the rest of the codebase and to prevent potential type resolution issues.
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
| // Website: https://www.blazor.zone or https://argozhang.github.io/ | ||
|
|
||
| namespace BootstrapBlazor.Components.DataConverter; |
There was a problem hiding this comment.
suggestion: Namespace 'BootstrapBlazor.Socket.DataConverter' is singular, while others use plural.
Consider renaming the namespace to 'BootstrapBlazor.Socket.DataConverters' to match the convention used elsewhere in the codebase.
Suggested change
| namespace BootstrapBlazor.Components.DataConverter; | |
| namespace BootstrapBlazor.Socket.DataConverters; |
|
|
||
| public ITcpSocketClient GetOrCreate(string name, Action<TcpSocketClientOptions> valueFactory) | ||
| { | ||
| if (!SocketLogging.Inited) |
There was a problem hiding this comment.
suggestion (bug_risk): SocketLogging initialization is performed per GetOrCreate call.
This approach can cause redundant initialization and race conditions under concurrent access. Recommend initializing SocketLogging once at startup or ensuring thread safety.
Suggested implementation:
public DefaultTcpSocketFactory(IServiceProvider provider)
{
this.provider = provider;
// Ensure SocketLogging is initialized only once at startup
if (!SocketLogging.Inited)
{
var logger = provider.GetService<ILogger<DefaultTcpSocketFactory>>();
if (logger != null)
{
SocketLogging.Init(logger);
}
}
}
public ITcpSocketClient GetOrCreate(string name, Action<TcpSocketClientOptions> valueFactory)
{
return _pool.GetOrAdd(name, key =>
{
var options = new TcpSocketClientOptions();- You may need to ensure that the
providerfield is defined at the class level and properly assigned in the constructor. - If there are multiple constructors, make sure SocketLogging initialization is performed in all relevant ones.
- Remove any other redundant SocketLogging initialization from elsewhere in the class.
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
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.
Link issues
fixes #555
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a SocketLogging utility and integrate it into TCP socket factory and data conversion components while reorganizing project namespaces under BootstrapBlazor.Socket for consistency
New Features:
Enhancements: