-
Notifications
You must be signed in to change notification settings - Fork 253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: removes the Tree
method from the NMT wrapper and introduces the ProveRange
API
#1438
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1438 +/- ##
==========================================
- Coverage 49.22% 49.18% -0.05%
==========================================
Files 79 79
Lines 4477 4477
==========================================
- Hits 2204 2202 -2
- Misses 2089 2091 +2
Partials 184 184
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm I couldn't find anywhere where this is used, and I don't remember why this was added in the first place, so it makes sense to remove it to me!
good find!
Indeed! there was no significant use case for it. It was only utilized here to invoke the |
## Overview This PR addresses issue #1296 by adding the specification for the NMT wrapper. The specification is based on the state of implementation after the inclusion of the changes made in pull request #1438. ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords
Overview
While reviewing the NMT wrapper implementation and attempting to write its specs, came across a contradiction in the wrapper's design. On one hand, the wrapper is intended to encapsulate an instance of NMT and re-define/modify some of its APIs, specifically the
Push
method. On the other hand, the wrapper also exposes theTree()
method, which returns a pointer to the underlying NMT tree and thus allows direct access to the tree and enables arbitrary manipulation of the underlying tree outside of the intended control of the wrapper. For instance, one can bypass the NMT wrapperPush
method and directly call thePush
method of the underlying tree using.Tree().Push()
. This not only widens the error surface but also violates the initial objective of the wrapper.To address this issue, the
Tree()
API has been removed, and all necessary methods of the underlying NMT are now exposed through the wrapper directly.Checklist