Skip to content

Conversation

KSemenenko
Copy link
Member

Description

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Performance improvement
  • Code refactoring

Changes Made

Testing

  • Unit tests pass
  • Integration tests pass
  • Manual testing completed

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Related Issues

Closes #

Screenshots (if applicable)

Additional Notes

@KSemenenko KSemenenko requested a review from Copilot August 20, 2025 10:05
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces comprehensive feedback improvements to the ManagedCode.Communication library, focusing on enhanced logging capabilities, command idempotency features, improved JSON serialization, and modernized API design.

  • Updates .NET target framework from multiple versions (6.0, 7.0, 8.0) to .NET 9.0 exclusively
  • Introduces high-performance logging with source generators and centralized logger configuration
  • Implements comprehensive command idempotency system with memory and Orleans store implementations

Reviewed Changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
README.md Updates .NET version badge and adds logging configuration documentation
ManagedCode.Communication/ResultT/Result.cs Adds JSON serialization attributes and new validation properties
ManagedCode.Communication/Result/Result.cs Adds IsValid property and InvalidObject for JSON serialization
ManagedCode.Communication.csproj Adds Microsoft.Extensions.Caching.Memory dependency for command features
ManagedCode.Communication/Logging/* Introduces high-performance logging with source generators
ManagedCode.Communication/Commands/* Implements comprehensive command idempotency system
ManagedCode.Communication/IResult*.cs Enhances interfaces with JSON attributes and validation properties
ManagedCode.Communication.AspNetCore/* Modernizes service registration and filter configuration
ManagedCode.Communication.Orleans/* Updates Orleans store with new idempotency interface methods
Test files Comprehensive test coverage for new features
Comments suppressed due to low confidence (1)

ManagedCode.Communication/Commands/Extensions/CommandIdempotencyExtensions.cs:140

  • The magic numbers 0.8 and 0.4 for jitter calculation should be extracted as named constants to improve code readability and maintainability.
        this ICommandIdempotencyStore store,

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

_lastResortLoggerFactory ??= LoggerFactory.Create(builder =>
builder.SetMinimumLevel(LogLevel.Warning));

return new Logger<Result>(_lastResortLoggerFactory);
Copy link
Preview

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a new LoggerFactory with every call when _lastResortLoggerFactory is null could be inefficient. Consider using a singleton pattern or lazy initialization to ensure the factory is created only once.

Suggested change
return new Logger<Result>(_lastResortLoggerFactory);
// Use thread-safe lazy initialization for the last resort logger factory
return new Logger<Result>(_lastResortLoggerFactory.Value);

Copilot uses AI. Check for mistakes.

@KSemenenko KSemenenko assigned Copilot and KSemenenko and unassigned Copilot Aug 22, 2025
# Conflicts:
#	ManagedCode.Communication/ResultT/Result.cs
@KSemenenko
Copy link
Member Author

@nandos13 what do you think about this one?

I want to add interface to sstandartize all method like From and etc, to do not forgot to implement them.
also better logger, and other stuff

@nandos13
Copy link
Contributor

@nandos13 what do you think about this one?

I want to add interface to sstandartize all method like From and etc, to do not forgot to implement them. also better logger, and other stuff

@KSemenenko It would be great to leverage static interface members here. Again, this could really reduce duplicated code and make it much easier to add new overloads. I mocked up the idea below:

public interface IResultFactory<TSelf>
    where TSelf : IResultFactory<TSelf>
{
    // Contract only requires these two methods to be implemented by the type
    static abstract TSelf Succeed();
    static abstract TSelf Fail(Problem problem);

    // Everything else can be implemented on the interface, since they are the same for all types.
    // Eg. This method creates a generic error Problem instance,
    // then pass it along to the abstract Fail method above.
    static TSelf Fail()
    {
        var problem = Problem.GenericError();
        return TSelf.Fail(problem);
    }
}

Now, the Result types just need to implement the interface like this:

public partial struct Result : IResultFactory<Result>
{
    public static Result Succeed()
    {
        return CreateSuccess();
    }
    
    public static Result Fail(Problem problem)
    {
        return CreateFailed(problem);
    }
}

and the rest of the specific overloads (eg. Fail(string title), Fail(string title, string detail)) get moved to the interface itself & don't need to be implemented by all result types.

@KSemenenko
Copy link
Member Author

Thanks for feedback @nandos13
This is nice idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants