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

Refactoring of the TriblerNetworkRequest #7205

Merged
merged 15 commits into from
Jan 31, 2023

Conversation

drew2a
Copy link
Contributor

@drew2a drew2a commented Nov 29, 2022

Related to #7187

The main changes in this PR are the following:

  1. It fixes small logic mistakes in methods TriblerNetworkRequest.on_finish() and TriblerNetworkRequest.cancel()
  2. It removes automatic request addition in the constructor of a TriblerNetworkRequest. This change opens an opportunity to write correct tests for TriblerNetworkRequest.

Minor changes:

  1. Renamed and replaced TriblerNetworkRequest and TriblerRequestManager.
  2. Extended logging.

The PR looks huge but it is mostly because the way how a TriblerNetworkRequest should be added to TriblerRequestManager has been changed.

From

Request()

To

request = Request()
manager.add(request)

@drew2a drew2a added this to the 7.14.0 milestone Nov 29, 2022
@drew2a drew2a force-pushed the refactoring/network_request branch 2 times, most recently from cc7dfe7 to 304cebf Compare November 29, 2022 11:08
@ichorid
Copy link
Contributor

ichorid commented Nov 29, 2022

For the love of Guido, why not just manager.add(Request())?! 🤯

@drew2a drew2a force-pushed the refactoring/network_request branch from 4ab0be8 to f8ebc1b Compare January 19, 2023 12:55
@drew2a drew2a changed the title WIP: Refactoring of the TriblerNetworkRequest Refactoring of the TriblerNetworkRequest Jan 19, 2023
@drew2a
Copy link
Contributor Author

drew2a commented Jan 20, 2023

@ichorid

For the love of Guido, why not just manager.add(Request())?! 🤯

There are always several ways of writing code.

I used this example above because it better describes the actual change.

And anticipating your next question. This is:

        request = Request(
            endpoint="ipv8/asyncio/drift",
            on_finish=self.on_ipv8_health_enabled,
            data={
                "enable": True
            },
            method=Request.PUT
        )

        request_manager.add(request)

better than:

        request_manager.add(
            Request(
                endpoint="ipv8/asyncio/drift",
                on_finish=self.on_ipv8_health_enabled,
                data={
                    "enable": True
                },
                method=Request.PUT
            )
        )

@ichorid
Copy link
Contributor

ichorid commented Jan 20, 2023

Even in cases like this?

        request = Request("statistics/tribler", self.on_tribler_statistics)
        request_manager.add(request)

vs

        request_manager.add(Request("statistics/tribler", self.on_tribler_statistics))

To me, the second one reads almost like natural text.

@drew2a
Copy link
Contributor Author

drew2a commented Jan 20, 2023

Even in cases like this?

Yes.

@drew2a drew2a force-pushed the refactoring/network_request branch from 0644084 to 92c36a4 Compare January 20, 2023 18:00
@drew2a drew2a marked this pull request as ready for review January 20, 2023 18:17
@drew2a drew2a requested review from a team and kozlovsky and removed request for a team January 20, 2023 18:17
src/tribler/gui/network/request_manager.py Outdated Show resolved Hide resolved
src/tribler/gui/widgets/downloadspage.py Show resolved Hide resolved
src/tribler/gui/network/request_manager.py Outdated Show resolved Hide resolved
src/tribler/gui/network/request_manager.py Outdated Show resolved Hide resolved
src/tribler/gui/network/request_manager.py Outdated Show resolved Hide resolved
src/tribler/gui/network/request_manager.py Outdated Show resolved Hide resolved
src/tribler/gui/widgets/channelcontentswidget.py Outdated Show resolved Hide resolved
src/tribler/gui/network/request/file_download_request.py Outdated Show resolved Hide resolved
src/tribler/gui/widgets/downloadspage.py Show resolved Hide resolved
src/tribler/gui/network/request/shutdown_request.py Outdated Show resolved Hide resolved
@drew2a drew2a requested a review from kozlovsky January 30, 2023 14:10
@drew2a drew2a force-pushed the refactoring/network_request branch from 897b1e8 to 518a1d1 Compare January 30, 2023 14:11
@drew2a
Copy link
Contributor Author

drew2a commented Jan 30, 2023

@kozlovsky thank you for the detailed feedback and for refining the code 🤝

@drew2a drew2a force-pushed the refactoring/network_request branch from 30801e7 to 7bcf112 Compare January 30, 2023 16:56
@drew2a drew2a merged commit cf5d52b into Tribler:main Jan 31, 2023
@drew2a drew2a modified the milestones: 7.14.0, 7.13.0 Mar 15, 2023
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.

3 participants