-
Notifications
You must be signed in to change notification settings - Fork 284
Conversation
|
||
class AppsManager(object): | ||
""" Temporary solution for apps detection and management. """ | ||
def __init__(self): | ||
self.apps = OrderedDict() | ||
self.task_types = dict() |
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.
Please merge b0.20 into develop and add type hints here.
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.
that's a bit more involved problem ;)
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.
You'll have conflicts to resolve, because I added a ton of type hints in this file in b0.20 :)
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.
yeah, and there are more conflicts between b0.20 and develop already... I don't think it's a problem pertaining to this particular pull request
Co-Authored-By: Adam Mizerski <amizerski@golem.network>
@@ -117,39 +117,57 @@ def add_new_task(task, *_args, **_kwargs): | |||
), | |||
) | |||
class TestCreateTask(ProviderBase, TestClientBase): | |||
@staticmethod | |||
def _get_task_dict(**data): | |||
task_dict = dummytaskstate.DummyTaskDefinition().to_dict() |
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 probably should look like this:
task_dict = dummytask.DummyTaskBuilder.build_dictionary(dummytaskstate.DummyTaskDefinition())
because build_dictionary
adds subtask_data_size
and difficulty
to dict.
Maybe not required, if tests are working without this. Dunno.
See also #2424
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 definitely against adding
TaskTypeInfo.id
. See comments I added earlier.
@etam what is the problem with using a normalized form for comparisons?
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 definitely against adding TaskTypeInfo.id
. See comments I added earlier.
In I can approve this, because it's about concent and blender, not about task type names. This can be cleaned up in a separate issue. |
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 not quite happy about it, but whatever. Let's clean this case-sensitivity stuff elsewhere.
Codecov Report
@@ Coverage Diff @@
## develop #4647 +/- ##
===========================================
+ Coverage 88.65% 90.25% +1.59%
===========================================
Files 224 224
Lines 20206 20219 +13
===========================================
+ Hits 17914 18248 +334
+ Misses 2292 1971 -321 |
closes: #4546