zhongjiajie commented on code in PR #130: URL: https://github.com/apache/dolphinscheduler-sdk-python/pull/130#discussion_r1441206469
########## UPDATING.md: ########## @@ -27,6 +27,8 @@ It started after version 2.0.5 released * Remove attribute tenant from pydolphinscheduler.core.workflow.workflow ([#54](https://github.com/apache/dolphinscheduler-sdk-python/pull/54)) and please change tenant name in ``config.yaml`` in ``PYDS_HOME`` * Drop support of python3.6 and python3.7 ([#126](https://github.com/apache/dolphinscheduler-sdk-python/pull/126)) +* Deprecated old format of `http_params` in the Http class to support a new format. Users should now use the updated format.([#88])([#130])(https://github.com/apache/dolphinscheduler-sdk-python/pull/130) Review Comment: ```suggestion * Change parameter `http_params` to dict type for easy to use in task http.([#130](https://github.com/apache/dolphinscheduler-sdk-python/pull/130)) ``` ########## tests/tasks/test_http.py: ########## @@ -126,3 +127,30 @@ def test_http_get_define(): ): http = Http(name, url) assert http.task_params == expect_task_params + + +def test_http_params_conversion(): + # Create a sample http_params dictionary + http_params_dict = { + "prop1": "value1", + "prop2": "value2", + "prop3": "value3" + } + + # Create an instance of the Http class with http_params as a dictionary + http_instance = Http( + name="test_http", + url="http://www.example.com";, + http_method="GET", + http_params=http_params_dict + ) + + # Print the initialized http_params attribute (should be converted to the desired format) + print("Converted http_params:", http_instance.http_params) + + # Add any assertions or additional tests as required based on your project's logic + assert isinstance(http_instance.http_params, list) + assert len(http_instance.http_params) == len(http_params_dict) Review Comment: we should also add one more test for `http_instance.http_params`, cause this is the core concept about what we change in this PR. For example ```py expect = [ {"prop": "prop1", "direct": "IN", "type": "VARCHAR", "value": "value1"}, {"prop": "prop2", "direct": "IN", "type": "VARCHAR", "value": "value2"}, {"prop": "prop3", "direct": "IN", "type": "VARCHAR", "value": "value3"}, ] assert http.http_params == expect ``` ########## tests/tasks/test_http.py: ########## @@ -126,3 +127,30 @@ def test_http_get_define(): ): http = Http(name, url) assert http.task_params == expect_task_params + + +def test_http_params_conversion(): + # Create a sample http_params dictionary + http_params_dict = { + "prop1": "value1", + "prop2": "value2", + "prop3": "value3" + } + + # Create an instance of the Http class with http_params as a dictionary + http_instance = Http( + name="test_http", + url="http://www.example.com";, + http_method="GET", + http_params=http_params_dict + ) + + # Print the initialized http_params attribute (should be converted to the desired format) + print("Converted http_params:", http_instance.http_params) Review Comment: Maybe we can remove those two lines, it is useless for our tests ########## src/pydolphinscheduler/tasks/http.py: ########## @@ -51,7 +53,29 @@ class HttpCheckCondition: class Http(Task): - """Task HTTP object, declare behavior for HTTP task to dolphinscheduler.""" + """Task HTTP object, declare behavior for HTTP task to dolphinscheduler. + Attributes: + _task_custom_attr (set): A set containing custom attributes specific to the Http task, + including 'url', 'http_method', 'http_params', and more. + Args: + :param str name: The name or identifier for the HTTP task. + :param str url: The URL endpoint for the HTTP request. + :param str http_method: The HTTP method for the request (GET, POST, etc.). Defaults to HttpMethod.GET. + :param dict http_params: Parameters for the HTTP request. Defaults to None. + :param str http_check_condition: Condition for checking the HTTP response status. + Defaults to HttpCheckCondition.STATUS_CODE_DEFAULT. + :param str condition: Additional condition to evaluate if `http_check_condition` is not STATUS_CODE_DEFAULT. + :param int connect_timeout: Connection timeout for the HTTP request in milliseconds. Defaults to 60000. + :param int socket_timeout: Socket timeout for the HTTP request in milliseconds. Defaults to 60000. Review Comment: ```suggestion :param name: The name or identifier for the HTTP task. :param url: The URL endpoint for the HTTP request. :param http_method: The HTTP method for the request (GET, POST, etc.). Defaults to HttpMethod.GET. :param http_params: Parameters for the HTTP request. Defaults to None. :param http_check_condition: Condition for checking the HTTP response status. Defaults to HttpCheckCondition.STATUS_CODE_DEFAULT. :param condition: Additional condition to evaluate if `http_check_condition` is not STATUS_CODE_DEFAULT. :param connect_timeout: Connection timeout for the HTTP request in milliseconds. Defaults to 60000. :param socket_timeout: Socket timeout for the HTTP request in milliseconds. Defaults to 60000. Attributes: _task_custom_attr (set): A set containing custom attributes specific to the Http task, including 'url', 'http_method', 'http_params', and more. ``` ########## tests/tasks/test_http.py: ########## @@ -126,3 +127,30 @@ def test_http_get_define(): ): http = Http(name, url) assert http.task_params == expect_task_params + + +def test_http_params_conversion(): Review Comment: BTW we should add one more test for the warning when user still pass list to http task parameter `http_params`. we have an example in https://github.com/apache/dolphinscheduler-sdk-python/blob/f82e07d232380fe9f5582db4dcca920ae00befd4/tests/tasks/test_sub_workflow.py#L110-L136 wish it can help you ########## tests/tasks/test_http.py: ########## @@ -126,3 +127,30 @@ def test_http_get_define(): ): http = Http(name, url) assert http.task_params == expect_task_params + + +def test_http_params_conversion(): Review Comment: due to you already copy and paste it from file `test_http_conversion.py` I thing you can remove the file `test_http_conversion.py`. ########## tests/tasks/test_http.py: ########## @@ -126,3 +127,30 @@ def test_http_get_define(): ): http = Http(name, url) assert http.task_params == expect_task_params + + +def test_http_params_conversion(): + # Create a sample http_params dictionary + http_params_dict = { + "prop1": "value1", + "prop2": "value2", + "prop3": "value3" + } + + # Create an instance of the Http class with http_params as a dictionary + http_instance = Http( Review Comment: ```suggestion http = Http( ``` ########## tests/tasks/test_http.py: ########## @@ -126,3 +127,30 @@ def test_http_get_define(): ): http = Http(name, url) assert http.task_params == expect_task_params + + +def test_http_params_conversion(): + # Create a sample http_params dictionary + http_params_dict = { + "prop1": "value1", + "prop2": "value2", + "prop3": "value3" Review Comment: it is better to add more value type for our test ```suggestion "prop1": "value1", "prop2": 135, "prop3": "value3" ``` -- 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]
