feat: add hook data #1587
feat: add hook data #1587
Conversation
Signed-off-by: mdxabu <[email protected]>
…d ConcurrentHashMap and use a synchronized HashMap instead Signed-off-by: mdxabu <[email protected]>
Signed-off-by: mdxabu <[email protected]>
Co-authored-by: alexandraoberaigner <[email protected]> Signed-off-by: Simon Schrottner <[email protected]>
Co-authored-by: alexandraoberaigner <[email protected]> Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Co-authored-by: chrfwow <[email protected]> Signed-off-by: Abu <[email protected]>
Co-authored-by: chrfwow <[email protected]> Signed-off-by: Abu <[email protected]>
Signed-off-by: Alexandra Oberaigner <[email protected]>
alexandraoberaigner
force-pushed
the
feat/hook-data
branch
from
8aca6b6 to
81f2e4f
Compare
Signed-off-by: Simon Schrottner <[email protected]>
aepfli
force-pushed
the
feat/hook-data
branch
from
4c4e7c3 to
6d4f700
Compare
Signed-off-by: Simon Schrottner <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1587 +/- ##
============================================
+ Coverage 92.82% 93.00% +0.18%
- Complexity 487 504 +17
============================================
Files 46 50 +4
Lines 1170 1215 +45
Branches 103 106 +3
============================================
+ Hits 1086 1130 +44
- Misses 54 55 +1
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces HookData to allow hooks to maintain state across execution stages, which is a significant and valuable feature. The implementation refactors HookContext into an interface with different implementations for contexts with and without hook data, updates HookSupport to manage data isolation, and adds comprehensive tests. The changes are well-structured and thoughtful. I've provided a few suggestions to improve API design and test robustness. Overall, this is a solid contribution towards the feature, acknowledging its work-in-progress status.
Signed-off-by: Simon Schrottner <[email protected]>
| @@ -54,6 +54,30 @@ public Optional<EvaluationContext> before(HookContext<Object> ctx, Map<String, O | |||
| return Optional.ofNullable(new ImmutableContext()); | |||
| } | |||
| }); | |||
| client.addHooks(new Hook<String>() { | |||
There was a problem hiding this comment.
Doesn't this have to be a StringHook for the filter to work?
Signed-off-by: Simon Schrottner <[email protected]>
aepfli
force-pushed
the
feat/hook-data
branch
from
14638db to
c3f8f36
Compare
|
/Gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a HookData mechanism, allowing hooks to maintain state across execution stages for a single flag evaluation. This is a significant and well-implemented feature. The changes involve refactoring HookContext into an interface and providing implementations for contexts with and without hook-specific data. The core logic in HookSupport and OpenFeatureClient is updated to manage and pass this data correctly, ensuring isolation between hooks and evaluations. The addition of a Pair utility class and new tests are also noted.
My review focuses on improving code clarity and robustness. I've suggested a change to the Pair class for naming consistency and a modification to HookData's default implementation to make it simpler and safer. Overall, this is a great contribution that enhances the hook system's capabilities.
|
This PR
improves PR #1506 from @mdxabu who started the work on adding hook data
Related Issues
Fixes #1472
Notes
Tests & Impl are still WIP: