Skip to content
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 the IPv8 pointer #7620

Closed
wants to merge 2 commits into from
Closed

Conversation

qstokkink
Copy link
Contributor

@qstokkink qstokkink commented Oct 4, 2023

This PR upgrades Tribler to the latest IPv8 version.

@qstokkink qstokkink force-pushed the ipv8_pointer_update branch 12 times, most recently from f6e0649 to 48b860c Compare October 5, 2023 12:24
@qstokkink qstokkink force-pushed the ipv8_pointer_update branch from 48b860c to e81c53b Compare October 5, 2023 13:09
@qstokkink qstokkink changed the title [DO NOT MERGE] Updated the IPv8 pointer Updated the IPv8 pointer Oct 5, 2023
@qstokkink qstokkink force-pushed the ipv8_pointer_update branch 3 times, most recently from 02f61de to 35d81ca Compare October 5, 2023 13:57
@qstokkink qstokkink marked this pull request as ready for review October 5, 2023 14:05
@qstokkink qstokkink requested review from a team and drew2a and removed request for a team October 5, 2023 14:05
@qstokkink qstokkink force-pushed the ipv8_pointer_update branch from 35d81ca to dc55a18 Compare October 5, 2023 14:18
@qstokkink qstokkink force-pushed the ipv8_pointer_update branch from dc55a18 to 46e738c Compare October 5, 2023 14:22
@qstokkink
Copy link
Contributor Author

I'm not sure what is wrong with pytest / run (windows-latest).

Copy link
Contributor

@drew2a drew2a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great to see the pyipv8 version updated; thank you for keeping it current.

Regarding the other changes, I have some questions.

I'm not sure that introducing the CommunitySettings is an improvement. What is the necessity of this intermediate class, and what benefits does it provide over simply and directly passing arguments through the constructor?

Additionally, this PR introduces a new dependency (typing_extension). Could you elaborate on the advantages of using it and explain why native typing is insufficient?

@qstokkink
Copy link
Contributor Author

I'm not sure that introducing the CommunitySettings is an improvement. What is the necessity of this intermediate class, and what benefits does it provide over simply and directly passing arguments through the constructor?

This was introduced due to pylint/ruff's PLR0913 violation triggering on the IPv8 codebase. Ruff's documentation includes a small example of the benefits: https://docs.astral.sh/ruff/rules/too-many-arguments/ If you want a deeper Software Engineering argument, there is a more elaborate write-up here: https://matheus.ro/2018/01/29/clean-code-avoid-many-arguments-functions/

The necessity comes from the fact that Tribler will not work anymore with the latest version of IPv8 without this change. However, you can always hotpatch in some dirty construction later if you really want to. For this PR, I'll stick to the recommended approach.

Additionally, this PR introduces a new dependency (typing_extension). Could you elaborate on the advantages of using it and explain why native typing is insufficient?

typing_extensions is a collection of backports for native typing constructs but from future Python versions. So, in a sense, native typing IS sufficient BUT we are running a Python version that is too old. IPv8 uses typing_extensions for the backports of Self and Protocol.

@drew2a
Copy link
Contributor

drew2a commented Oct 6, 2023

Settings

Too many arguments in a function can sometimes be a signal, or 'smell,' of suboptimal code or design. While this might be true in some instances, it's not universally applicable. The number of function parameters should not serve as an absolute indicator necessitating refactoring.

In Tribler, some communities do indeed have numerous parameters, while others do not. Therefore, only a few communities in Tribler appear to be potential candidates for refactoring, not all of them.

In this context, I'd argue that the necessity for a specialized Setting object feels a bit like 'dictatorship' from the ipv8 side. Speaking as a developer who has crafted communities in Tribler and will continue to do so in the future, I find this approach unsavory as it introduces an unnecessary level of abstraction, will likely clutter the code, and doesn’t present any significant benefits.

Please consider amending ipv8 such that this feature becomes optional instead of mandatory.

typing_extensions

I understand. If 'IPv8 uses typing_extensions for the backports of Self and Protocol,' then IPv8 should install this dependency during its installation. It should not depend on the target application (Tribler) to install all required libraries.

@qstokkink
Copy link
Contributor Author

In Tribler, some communities do indeed have numerous parameters, while others do not. Therefore, only a few communities in Tribler appear to be potential candidates for refactoring, not all of them.

Community has 5 arguments to __init__, TriblerCommunity adds a 6th one. ALL communities are over the Pylint recommended max of 5 arguments before they even start adding anything.

I'd argue that the necessity for a specialized Setting object feels a bit like 'dictatorship' from the ipv8 side.

Calling an API a dictatorship seems a bit overly dramatic but I guess I can agree to being a dictator: I don't think it's a bad thing to have an API.

IPv8 should install this dependency during its installation. It should not depend on the target application (Tribler) to install all required libraries.

It does. However, Tribler redefines exact versions of dependencies that only appear in IPv8. I added this to be consistent.


I get the overall feeling that you are not favor of this PR, which is fine: I'm not here to force you to use anything and I'll just close it.

@qstokkink qstokkink closed this Oct 6, 2023
@qstokkink qstokkink deleted the ipv8_pointer_update branch September 2, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants