Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Jul 25, 2025

PR Type

Enhancement


Description

  • Add default implementations to interface methods

  • Throw NotImplementedException for unimplemented methods

  • Improve interface contract clarity


Diagram Walkthrough

flowchart LR
  A["IChatCompletion Interface"] --> B["GetChatCompletions Method"]
  A --> C["GetChatCompletionsAsync Method"]
  A --> D["GetChatCompletionsStreamingAsync Method"]
  B --> E["Default NotImplementedException"]
  C --> E
  D --> E
Loading

File Walkthrough

Relevant files
Enhancement
IChatCompletion.cs
Add default NotImplementedException to interface methods 

src/Infrastructure/BotSharp.Abstraction/MLTasks/IChatCompletion.cs

  • Added default implementations throwing NotImplementedException to
    three interface methods
  • Modified GetChatCompletions, GetChatCompletionsAsync, and
    GetChatCompletionsStreamingAsync methods
  • Provides explicit contract behavior for unimplemented methods
+3/-3     

@iceljc iceljc merged commit b3765c0 into SciSharp:master Jul 25, 2025
0 of 4 checks passed
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Interface Design

Adding default implementations to interface methods changes the contract behavior. This may break existing implementations that rely on compile-time enforcement of method implementation. Consider if this should be an abstract base class instead.

Task<RoleDialogModel> GetChatCompletions(Agent agent,
    List<RoleDialogModel> conversations) => throw new NotImplementedException();

Task<bool> GetChatCompletionsAsync(Agent agent, 
    List<RoleDialogModel> conversations, 
    Func<RoleDialogModel, Task> onMessageReceived,
    Func<RoleDialogModel, Task> onFunctionExecuting) => throw new NotImplementedException();

Task<RoleDialogModel> GetChatCompletionsStreamingAsync(Agent agent, 
    List<RoleDialogModel> conversations) => throw new NotImplementedException();
Runtime Behavior

The default NotImplementedException will cause runtime failures instead of compile-time errors. Existing code that doesn't implement these methods will now compile but fail at runtime, potentially making debugging harder.

    List<RoleDialogModel> conversations) => throw new NotImplementedException();

Task<bool> GetChatCompletionsAsync(Agent agent, 
    List<RoleDialogModel> conversations, 
    Func<RoleDialogModel, Task> onMessageReceived,
    Func<RoleDialogModel, Task> onFunctionExecuting) => throw new NotImplementedException();

Task<RoleDialogModel> GetChatCompletionsStreamingAsync(Agent agent, 
    List<RoleDialogModel> conversations) => throw new NotImplementedException();

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove default interface implementations

Adding default implementations to interface methods breaks the interface
contract and prevents proper polymorphism. Interface methods should remain
abstract to force implementing classes to provide their own implementations.
This change will cause runtime exceptions instead of compile-time errors when
methods aren't implemented.

src/Infrastructure/BotSharp.Abstraction/MLTasks/IChatCompletion.cs [18-27]

 Task<RoleDialogModel> GetChatCompletions(Agent agent,
-    List<RoleDialogModel> conversations) => throw new NotImplementedException();
+    List<RoleDialogModel> conversations);
 
 Task<bool> GetChatCompletionsAsync(Agent agent, 
     List<RoleDialogModel> conversations, 
     Func<RoleDialogModel, Task> onMessageReceived,
-    Func<RoleDialogModel, Task> onFunctionExecuting) => throw new NotImplementedException();
+    Func<RoleDialogModel, Task> onFunctionExecuting);
 
 Task<RoleDialogModel> GetChatCompletionsStreamingAsync(Agent agent, 
-    List<RoleDialogModel> conversations) => throw new NotImplementedException();
+    List<RoleDialogModel> conversations);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that adding default implementations which throw exceptions moves error detection from compile-time to runtime, which is a valid and important architectural concern.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant