-
Notifications
You must be signed in to change notification settings - Fork 452
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
Updated IPv8 while keeping Community kwargs #7645
Conversation
27c7caf
to
a400dd4
Compare
a400dd4
to
4be493a
Compare
4be493a
to
9e40f70
Compare
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.
Looks good
def args_kwargs_to_community_settings(settings_class, args, kwargs): | ||
return settings_class(my_peer=args[0] if len(args) > 0 else kwargs.pop("my_peer"), | ||
endpoint=args[1] if len(args) > 1 else kwargs.pop("endpoint"), | ||
network=args[2] if len(args) > 2 else kwargs.pop("network"), | ||
max_peers=args[4] if len(args) > 3 else kwargs.pop("max_peers", DEFAULT_MAX_PEERS), | ||
anonymize=args[5] if len(args) > 4 else kwargs.pop("anonymize", True), | ||
**kwargs) |
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 was wondering if it is possible to have a type-safe equivalent of args_kwargs_to_community_settings
that specifies the types of its arguments, and came up with the following:
community_class = TypeVar('community_class', bound=Community)
_missed = object()
def create_settings(cls: Type[community_class],
my_peer: Peer = _missed,
endpoint: Endpoint = _missed,
network: Network = _missed,
max_peers: int = _missed,
anonymize: bool = _missed,
**kwargs
) -> community_class.settings_class:
params = dict(my_peer=my_peer, endpoint=endpoint, network=network,
max_peers=max_peers, anonymize=anonymize)
params.update(kwargs)
params = {key: value for key, value in params.items() if value is not _missed}
return cls.settings_class(**params)
def create_community(cls: Type[community_class],
my_peer: Peer = _missed,
endpoint: Endpoint = _missed,
network: Network = _missed,
max_peers: int = _missed,
anonymize: bool = _missed,
**kwargs
) -> community_class:
return cls(create_settings(cls, my_peer, endpoint, network, max_peers, anonymize, **kwargs))
With this, instead of
community = DiscoveryCommunity(args_kwargs_to_community_settings(DiscoveryCommunity.settings_class,
[self.peer, ipv8.endpoint, ipv8.network],
{"max_peers": 100}))
It is possible to write
community = create_community(DiscoveryCommunity, self.peer, ipv8.endpoint, ipv8.network, max_peers=100)
and argument types will be correctly checked.
But as args_kwargs_to_community_settings
is used in only a few places, I'm not advocating for a type-safe version; the current PR version looks good enough to me
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.
@kozlovsky That seems like a nice improvement. Would you be O.K. with opening a PR for your proposal after this PR has been merged?
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.
@qstokkink Sure!
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.
@kozlovsky thanks! I'll merge this PR in its current state then.
This PR is the second attempt to update the IPv8 version to
2.12
in Tribler.The previous PR to update the version, #7620, was rejected because communities could not
__init__
with kwargs. In this PR, I introduce adapters to allow communities to__init__
withargs
andkwargs
, like in the previous IPv8 version (2.11
).TriblerMockIPv8
provides the old interface using the newMockIPv8
.TriblerTestBase
provides the old interface using the newTestBase
.TriblerCommunity
provides the old interface using the newCommunity
.Disclaimer: I still prefer the implementation of #7620 over this.