Wrap conditions and context in newtypes #33
Conversation
Codecov Report
@@ Coverage Diff @@
## master #33 +/- ##
==========================================
+ Coverage 15.61% 16.12% +0.51%
==========================================
Files 11 12 +1
Lines 429 465 +36
==========================================
+ Hits 67 75 +8
- Misses 362 390 +28
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
fjarri
changed the title
| def __init__(self, conditions: str): | ||
| ... | ||
|
|
||
| @classmethod | ||
| def from_string(cls, conditions: str) -> Conditions: |
There was a problem hiding this comment.
What is the difference between these two constructors?
There was a problem hiding this comment.
There's a difference in Rust (allocating vs non-allocating), and this structure is just mapped here. I guess we could safely remove it from Python and JS bindings.
| pub struct Conditions(String); | ||
|
|
||
| impl Conditions { | ||
| /// Creates a new conditions object. |
There was a problem hiding this comment.
Is this comment likely to be helpful?
There was a problem hiding this comment.
I don't really have any better ideas. We could say here that it's supposed to be a JSON string, but nothing in nucypher-core really acts on that assumption...
There was a problem hiding this comment.
But do we need any comment to explain that new makes a new object?
There was a problem hiding this comment.
We do because of #![warn(missing_docs)]. It's easier to just write something than to add a specific exclusion in this case.
ContextandConditionsnewtypes.MessageKit,RetrievalKitandReencryptionRequestAsBackendandFromBackendand replace them with derivedAsRefandFrom)Note: because of a strange quirk of wasm-bindgen (rustwasm/wasm-bindgen#2370) constructors taking
Option<Conditions>orOption<Context>consume their arguments in JS. This is already an issue in Umbral (nucypher/rust-umbral#25).