feat: C local bucketing scaffolding #194
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the local bucketing interface by renaming GetVariableForUserProtobuf to GetVariable and introduces a new CLocalBucketing class implementation. The rename creates a method overload alongside the existing string-based GetVariable method.
- Renamed
GetVariableForUserProtobuftoGetVariablefor cleaner API naming - Added
CLocalBucketingclass as a stub implementation ofILocalBucketing - Enhanced exception handling to support fallback behavior when protobuf method is not implemented
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| WASMLocalBucketing.cs | Renamed method from GetVariableForUserProtobuf to GetVariable |
| ILocalBucketing.cs | Updated interface to replace old method signature with new GetVariable overload |
| DevCycleLocalClient.cs | Updated all callers to use renamed method, added NotImplementedException handling, and minor style improvements |
| CLocalBucketing.cs | New stub implementation of ILocalBucketing with all methods throwing NotImplementedException |
Comments suppressed due to low confidence (1)
DevCycle.SDK.Server.Local/Api/DevCycleLocalClient.cs:340
- The variable
existingVariableis declared but never initialized before the fallthrough path at line 369. While the code works because line 369 doesn't use it, this creates confusion. Consider removing this declaration and declaring the variable inside the try block at line 356 where it's actually used and returned.
Variable<T> existingVariable;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catch (NotImplementedException) | ||
| { | ||
| return Task.FromResult(Common.Model.Variable<T>.InitializeFromVariable(null, key, defaultValue, DefaultReasonDetails.UserNotTargeted)); | ||
| // C Local bucketing |
There was a problem hiding this comment.
The catch block for NotImplementedException is empty with only a comment indicating 'C Local bucketing'. After catching this exception, the code falls through to line 369 which returns a default variable with DefaultReasonDetails.Error. This flow is unclear. If this is intentional fallback behavior for C Local bucketing, consider either: (1) adding an explicit comment explaining why the error default is returned, or (2) implementing the C Local bucketing path here if it differs from the error case.
| // C Local bucketing | |
| // C Local bucketing is not implemented yet. | |
| // Fall through to return the error default below. |
| catch (Exception e) | ||
| { | ||
| beforeError = e; | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
Scaffolding for the C local bucketing library.
Need to add handling/fallback if the bucketing library is set to not use PB.