zhongjiajie commented on code in PR #53:
URL:
https://github.com/apache/dolphinscheduler-sdk-python/pull/53#discussion_r1052885382
##########
src/pydolphinscheduler/core/task.py:
##########
@@ -169,6 +195,18 @@ def __init__(
self.workflow = kwargs.pop("process_definition")
else:
self.workflow: Workflow = workflow or WorkflowContext.get()
+
+ if "local_params" in kwargs:
Review Comment:
we should directly remove the `local_params` in function `__init__` in L162,
otherwise local_params will never in `kwargs`
##########
src/pydolphinscheduler/core/task.py:
##########
@@ -82,7 +83,28 @@ def __hash__(self):
class Task(Base):
- """Task object, parent class for all exactly task type."""
+ """Task object, parent class for all exactly task type.
+
+ :param name: The name of the task. Node names within the same workflow
must be unique.
+ :param task_type: task type
+ :param description: str, Describing the function of this node.
+ :param flag: str, default TaskFlag.YES,
Review Comment:
we should remove the type here, because the parameter type will auto parser
from class function `__init__` type hint from code, you can see in
https://dolphinscheduler.apache.org/python/main/api.html#pydolphinscheduler.tasks.DataX
```suggestion
:param flag: default TaskFlag.YES,
```
##########
src/pydolphinscheduler/core/yaml_workflow.py:
##########
@@ -75,6 +76,24 @@ def parse_string_param_if_config(string_param: str,
**kwargs):
return string_param
+ @staticmethod
+ def parse_string_param_if_parameter(string_param: str, **kwargs):
+ """Use TYPE(value) to set local params."""
+ key_path = kwargs.get("key_path")
+ if key_path.split("-")[0] not in {"input_params", "output_params"}:
Review Comment:
can we add `-` to `pydolphinscheduler.constants.Symbol` ?
##########
src/pydolphinscheduler/core/task.py:
##########
@@ -169,6 +195,18 @@ def __init__(
self.workflow = kwargs.pop("process_definition")
else:
self.workflow: Workflow = workflow or WorkflowContext.get()
+
+ if "local_params" in kwargs:
Review Comment:
also for L226, and when we remove both of them, we should add attr check in
function `local_params`, because `self._local_params` only assign in L206 when
`local_params` in `kwargs`, means it is optional, so we should check wether it
exists or not first
```diff
- local_params = copy.deepcopy(self._local_params)
+ local_params = copy.deepcopy(self._local_params) if hasattr(self,
"_local_params") or []
```
##########
docs/source/concept.rst:
##########
@@ -260,10 +260,21 @@ After that, we could see new file named
``bare-create.py`` is be created in reso
Both parameter ``resource_list`` in workflow and task is list of string
which mean you could upload and reference
multiple files. For more complex usage, you could read
:doc:`howto/multi-resources`.
+Define Local Parameters
+-----------------------
+
+There are two ways to define local parameters
Review Comment:
could we add more detail about what parameters is, and what it works for?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
