-
Notifications
You must be signed in to change notification settings - Fork 516
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
Provide an optional Profile
to the verification key strategy
#2265
Provide an optional Profile
to the verification key strategy
#2265
Conversation
Signed-off-by: Sacha Kozma <[email protected]>
Signed-off-by: Sacha Kozma <[email protected]>
Profile
to the verification key strategy
Signed-off-by: Sacha Kozma <[email protected]>
Given this is related to #2235 -- do we need this one to be merged as part of 0.8.2? @dbluhm @usingtechnology , could you please take a look at this one and let us know? Thanks! |
self, | ||
did: str, | ||
profile: Optional[Profile] = None, |
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.
I think it would be better to require the profile and just not use it if the strategy does not require it. This will make the calls to the strategy more consistent and prevent intermittent failures from one call not having it but others do.
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.
I agree with @dbluhm.
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.
Done
Yes, ideally this is merged with 0.8.2 if we can swing it. |
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.
I agree that the profile should NOT be optional and let the implementations decide if it needs to use it.
Also, you need to update all the docstrings with the new parameter.
self, | ||
did: str, | ||
profile: Optional[Profile] = None, |
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.
I agree with @dbluhm.
Signed-off-by: Sacha Kozma <[email protected]>
Signed-off-by: Sacha Kozma <[email protected]>
0cba53e
to
2ed81c1
Compare
Kudos, SonarCloud Quality Gate passed! |
PR #2235 has just been merged, but I realized that not having access to
Profile
could be too restrictive. For example, accessing storage with the current function's signature is hard. In fact, I thought that custom strategies could be initialized with anInjectionContext
and query what they need, but it doesn't seem to be working. Let me know if there is a more idiomatic way of doing this.This PR fixes this by providing an optional
Profile
and switching to anasync
method.