Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Feature] Add
BasicClientProtocol
andAdaptiveDriftConstrainedMixin
#384New 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
[Feature] Add
BasicClientProtocol
andAdaptiveDriftConstrainedMixin
#384Changes from all commits
ccaba39
e345284
febf990
9c45150
9bbdb97
81c1a41
3d02767
a8dc0de
e66fde4
9558d1d
6cf0340
0defd8e
2ce3083
01f00bf
06c08b6
cf5d8c4
6e0c353
629c5af
552a781
af052a5
560bf5a
a175745
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 you're going to get a linter error here for the f-string without a variable?
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.
For some reason, this passed our lint checks, but thanks for catching this. I've removed the f-string annotation.
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.
This is sort of a hard line to follow. I actually had to throw this through the debugger to get what was happening. Correct me if I'm wrong, but you're actually instantiating an object to extract its type here by using a string to construct the class name?
This might be the best way to do it (I haven't thought about it at all 😂) but thought I'd at least suggest taking two seconds to verify that it is ha. It sort of feels funny to me just because of the indirection.
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'm actually just creating a new class dynamically here. The thinking here is to provide a convenience method (or syntactic sugar) for the user to quickly adapt their clients with the mixins we support.
I've added a test
test_dynamically_created_class
that tests its usage. But basicallyThere 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.
It's a bit clearer now why you want to do this. It's still a difficult line to read/understand (the black formatting probably isn't helping ha). I'm fine with it as sugar.