-
Notifications
You must be signed in to change notification settings - Fork 259
Support the creation of a new mutable object with a pre-determined signature key #1245
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
Support the creation of a new mutable object with a pre-determined signature key #1245
Conversation
Previously `NodeMaker` always took responsibility for generating a keypair to use. Now the caller may supply one.
This gives the test suite access to the derivation function so it can re-derive certain values to use as expected results to compare against actual results.
and we always need overload now
I don't know why mypy fails to see it.
|
|
||
|
|
||
| def create_signing_keypair_from_string(private_key_der): | ||
| def create_signing_keypair_from_string(private_key_der: bytes) -> tuple[PrivateKey, PublicKey]: |
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.
There should be no change of behavior in this function. All of the changes are to fix complaints mypy has about this function now that it is annotated.
| """ | ||
| _validate_private_key(private_key) | ||
| return private_key.private_bytes( | ||
| return private_key.private_bytes( # type: ignore[attr-defined] |
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.
Try as I might, I could not convince mypy that the private_bytes method exists.
itamarst
left a comment
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.
Thank you, address and merge.
|
|
||
| T = TypeVar("T") | ||
|
|
||
| @overload |
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.
TIL
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.
So this is saying:
- If multiple is False, we return bytes
- If multiple is True, we return tuple of bytes
But then the final version allows returning random other things, regardless of value of multiple. So how does that work?
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.
You are close. The key is that `Literal[False] doesn't mean "False". It means a statically detectable False. For example:
get_arg(..., multiple=False)
or
x = False
get_arg(..., multiple=x)
but probably not
get_arg(..., multiple=True if x else False)
I don't know the exact extent to which mypy can track this down.
So the first two cases are for statically detectable False or True and the third case covers those and the case where the value couldn't be determined.
Writing this out and reconsidering it, I'm not sure how much sense it makes. I think I got lost in making mypy happy and wasn't thinking carefully enough about the real meaning. A couple things occur to me now:
- The
Tcase is just as possible for theLiteral[True]andLiteral[False]cases as for the third case. - The
Nonein the return type seems like nonsense. The only way to get None back is if that is the value given fordefault. It seems likeTshould cover this but maybe it doesn't because of some special cases applied toNone?
sigh... I'll see if I can work this into something more coherent.
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.
2490f0f fwiw
|
Unclear what the integration test failures are but I see them on the most recent master build too so I guess they aren't a problem with this branch. :/ |
For https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3962 in support of https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3961