Add check on infohash key existing #5910

drew2a · 2021-01-07T10:44:07Z

This PR resolves #5909 by adding a check on the existing of infohash key.

The root cause of the issue is too general an inaccurate design of ContentTableViewController.
My proposal: to schedule a refactoring for ContentTableViewController.

kozlovsky · 2021-01-07T10:54:13Z

src/tribler-gui/tribler_gui/widgets/triblertablecontrollers.py

@@ -132,6 +132,9 @@ def _on_selection_changed(self, selected, deselected):
 class HealthCheckerMixin:
    def check_torrent_health(self, data_item, forced=False):
        # TODO: stop triggering multiple checks over a single infohash by e.g. selection and click signals
+        if 'infohash' not in data_item:


As I understand it, channels have infohashes as well as regular torrents, but we probably don't want to show health information for channels in this view.

In other similar places, there is a check if data_item['type'] != REGULAR_TORRENT. I think we need to use a similar check here as well.

Checking infohash is a more reliable approach if we look at the change as a "bugfix".

But your suggestion is the more in "triblertablecontrollers' style". And your approach is more correct if we look at the change as a "triblertablecontrollers fix".

I've implemented your suggestion (still not sure that this is the best version of the bugfix).

It is indeed more correct to check for the entry type, not for its attributes. In this way, we catch the errors in the application logic, though at the cost of the thing crashing for the user.

kozlovsky · 2021-01-07T12:03:31Z

src/tribler-gui/tribler_gui/widgets/triblertablecontrollers.py

@@ -287,7 +293,7 @@ def add_menu_item(self, menu, name, item_index, callback):
    def selection_can_be_added_to_channel(self):
        for row in self.table_view.selectionModel().selectedRows():
            data_item = row.model().data_items[row.row()]
-            if 'type' in data_item and data_item['type'] in (REGULAR_TORRENT, CHANNEL_TORRENT, COLLECTION_NODE):
+            if dict_has(data_item, 'type', REGULAR_TORRENT, CHANNEL_TORRENT, COLLECTION_NODE):


Actually, I don't like this new dict_has function. In my opinion, it makes code much harder to understand.

Previously the code did not require additional context to understand what is going on. Now I need to look at the dict_has source to understand the details of its logic.

In the previous code it was clear to me that data_item is a dict instance (or, at least, some objects with the Mapping interface). Now the code will not break if data_item is, for example, None. So, if I need to extend this function in the future, I need to spend time to answer the question "can data_item be anything other than dict", whereas in the previous version it was obvious.

The new version is not significantly shorter. If the goal is to achieve brevity it was possible to write instead:

if data_item.get('type') in (REGULAR_TORRENT, CHANNEL_TORRENT, COLLECTION_NODE): ...

So I propose to roll back this change and remove the dict_has function.

Python is a dynamically typed language but it is strongly typed. The expressionfoo[bar] generates KeyError exception when it is incorrectly assumed that foo should be a mapping and bar is a required item of this mapping, and it is a good thing. It is like a programmer explicitly adds some assertions to its code which will guarantee that errors will raise fast and not be propagated to different places. When we add unnecessary checks if foo or if bar in foo we essentially convert strongly typed language to a weakly typed one.

Previously the code did not require additional context to understand what is going on. Now I need to look at the dict_has source to understand the details of its logic.

Yes, and this is ok. You need to understand once what this function does, and then relax for all other occurrences.
dict_has just extract a pattern that was used in this file three times

Now the code will not break if data_item is, for example, None

Yes, and this is great. No excuses for crash on the user side.

The new version is not significantly shorter.

Yes, but shortnesses is doubtful purpose. The new version reduces the number of repetitions (of a pattern itself and of key's value, inside the pattern). Therefore it decreases the chance to make a mistake.

dict_has just extract a pattern that was used in this file three times

Not all patterns probably should be extracted. a + b * c is better than addmul(a, b, c)

Yes, and this is great. No excuses for crash on the user side.

If we a sure that, say, something can never be None, adding unnecessary handling for None leads to less clear code, as from now it says to the future reader: there are some obscure cases when this variable really can be None.

Yes, and this is ok. You need to understand once what this function does, and then relax for all other occurrences.

Many developers need to do it, and even a single developer does not remember all details of all the small functions that he reads in the entire codebase and needs to read it again.

Actually, this specific case is small enough to turn it into the bikeshedding, but the general pattern looks a bit like a code obfuscation to me.

I agree with @kozlovsky . Python is not C++: Python programmers are used to some idiomatic ways to do simple stuff like checking dict attributes, etc. Python is not about inventing your own sub-language as C++ does. Python is about making the code immediately readable to anyone.

So, replacing an explicit one-liner with an implicit one-liner goes against the Zen of Python

import this 😉

Dijkstra would say that exceptions in reports can be used to show the presence of bugs, but never to show their absence :)

@ichorid

So, every release must look as stable as possible, even if it means hiding defects, right?

Probably it is better to just answer "yes" on your question.

But I would say, if a release doesn't look stable, you shouldn't publish it. Taking into account, that "obscure cases" always will be presented... we need to make release stable with the margin of safety.

(in my humble view)

Dijkstra would say that exceptions in reports can be used to show the presence of bugs, but never to show their absence :)

Of course. Yet exceptions is almost the only kind of feedback and support that we get from our userbase. Because, you know, we can't do telemetry.

kozlovsky · 2021-01-11T10:59:58Z

src/tribler-gui/tribler_gui/utilities.py

@@ -422,3 +422,9 @@ def disconnect(signal: pyqtSignal, callback: Callable):

 class CreationTraceback(Exception):
    pass
+
+
+def dict_has(d, key, *values_or):


Still think it is better to inline code without a function, but it is more important to have this fix in the release.
How about changing the function signature to

def dict_item_is_any_of(d, key, values):

Just a suggestion, as the values_or argument name requires to look at the function signature to understand the meaning of multiple values.

sonarcloud · 2021-01-11T11:17:31Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

drew2a requested review from xoriole and kozlovsky January 7, 2021 10:44

kozlovsky reviewed Jan 7, 2021

View reviewed changes

drew2a force-pushed the fix/5909 branch 3 times, most recently from 94fe4b1 to 88985ee Compare January 7, 2021 11:38

kozlovsky reviewed Jan 7, 2021

View reviewed changes

xoriole approved these changes Jan 11, 2021

View reviewed changes

Add check on infohash key existing

3f0adbb

kozlovsky approved these changes Jan 11, 2021

View reviewed changes

drew2a force-pushed the fix/5909 branch from 3720b7e to 50c627c Compare January 11, 2021 11:13

Refactor by implementing dict_item_is_any_of function

5e07056

drew2a force-pushed the fix/5909 branch from 50c627c to 5e07056 Compare January 11, 2021 11:14

drew2a merged commit 47dbcfd into Tribler:release-7.7 Jan 11, 2021

drew2a deleted the fix/5909 branch January 11, 2021 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check on infohash key existing #5910

Add check on infohash key existing #5910

drew2a commented Jan 7, 2021 •

edited

Loading

kozlovsky Jan 7, 2021

drew2a Jan 7, 2021 •

edited

Loading

ichorid Jan 7, 2021

kozlovsky Jan 7, 2021 •

edited

Loading

kozlovsky Jan 7, 2021

drew2a Jan 7, 2021 •

edited

Loading

kozlovsky Jan 7, 2021 •

edited

Loading

ichorid Jan 7, 2021

drew2a Jan 7, 2021

drew2a Jan 7, 2021 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

ichorid Jan 7, 2021

kozlovsky Jan 11, 2021

sonarcloud bot commented Jan 11, 2021

Add check on infohash key existing #5910

Add check on infohash key existing #5910

Conversation

drew2a commented Jan 7, 2021 • edited Loading

kozlovsky Jan 7, 2021

Choose a reason for hiding this comment

drew2a Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

ichorid Jan 7, 2021

Choose a reason for hiding this comment

kozlovsky Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

kozlovsky Jan 7, 2021

Choose a reason for hiding this comment

drew2a Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

kozlovsky Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

ichorid Jan 7, 2021

Choose a reason for hiding this comment

drew2a Jan 7, 2021

Choose a reason for hiding this comment

drew2a Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

ichorid Jan 7, 2021

Choose a reason for hiding this comment

kozlovsky Jan 11, 2021

Choose a reason for hiding this comment

sonarcloud bot commented Jan 11, 2021

drew2a commented Jan 7, 2021 •

edited

Loading

drew2a Jan 7, 2021 •

edited

Loading

kozlovsky Jan 7, 2021 •

edited

Loading

drew2a Jan 7, 2021 •

edited

Loading

kozlovsky Jan 7, 2021 •

edited

Loading

drew2a Jan 7, 2021 •

edited

Loading