Refactor Http class to utilize ParameterHelper for http_params conver…

HarshitNagpal29 commented 11 months ago

…sion

fix: #88

Brief Summary of The Change

Pull Request checklist

I confirm that the following checklist has been completed.

[x] Add/Change test cases for the changes.
[x] Add/Change the related documentation, should also change docs/source/config.rst when you change file default_config.yaml.
[ ] (Optional) Add your change to UPDATING.md when it is an incompatible change.

zhongjiajie commented 11 months ago

BTW, since it is an incompatible change, we should add some comments in https://github.com/apache/dolphinscheduler-sdk-python/blob/1b2ad576eef87430e0abee47cce60eec5a374475/UPDATING.md#L29 we already have some example in that.

And we better add some DeprecationWarning for the user who still passes old parameter type for it. the old parameter seems like

http = Http(
        name="http",
        url="http://www.google.com",
        http_method="GET",
        http_params=[
            {"prop": "abc", "httpParametersType": "PARAMETER", "value": "def"}
        ],
    )

And we should raise some warnings and tell users that the format is deprecation

https://github.com/apache/dolphinscheduler-sdk-python/blob/e1d15274a6ffc2cf2ee3236d3ba3e8a0b1cf70d3/src/pydolphinscheduler/core/task.py#L202-L207

maybe it can be

        if isinstance(http_params, list):
            warnings.warn(
                "The `http_params` parameter currently accept dict instead of list, you parameter is ignore.",
                DeprecationWarning,
            )

zhongjiajie commented 11 months ago

Hi @HarshitNagpal29, it is good to see you add some tests and fantastic docstring in this PR, well done 👍 +1 I had left some new comments for this PR, PTAL if you have time

HarshitNagpal29 commented 11 months ago

@zhongjiajie sir i have made all the changes as you mentioned.

HarshitNagpal29 commented 11 months ago

Hi @HarshitNagpal29, it is good to see you add some tests and fantastic docstring in this PR, well done 👍 +1 I had left some new comments for this PR, PTAL if you have time

sir please tell me what to do, i am happy to help

zhongjiajie commented 11 months ago

@zhongjiajie sir i am looking forward to participate in gsoc 2024 , i am new to open source contribution but i am well aware with javascript, node.js and somewhat python as well , i found your organisation pretty interesting and i am looking forward to contribute more, can you please provide some resources, places where i can connect with mentors more often , and some valuable feedback, i will be really grateful for that. thank you

Great, do you have any resources about gsoc 2024? our project join gsoc 2022 but not in 2023, and I have act as mentor for 2022 too. I want to know the timeline about project submission.

can you please provide some resources, places where i can connect with mentors more often

you can find my email in my GitHub profile and connect me via that.

zhongjiajie commented 11 months ago

BTW, we have a code style check for our codebase, you could run the command tox -e auto-lint to format the code. for more information, you can see in https://github.com/apache/dolphinscheduler-sdk-python/blob/main/CONTRIBUTING.md#code-style-using-tox

and for how to set tox development you can see in https://github.com/apache/dolphinscheduler-sdk-python/blob/main/CONTRIBUTING.md#automated-testing-with-tox

zhongjiajie commented 11 months ago

And I have commit and change some code to you branch, so you should pull them to your local before you add your change

HarshitNagpal29 commented 11 months ago

@zhongjiajie sir i have made the changes and added some tests as well suggested by you.

HarshitNagpal29 commented 11 months ago

@zhongjiajie sir i have mailed you to connect with you

HarshitNagpal29 commented 11 months ago

@zhongjiajie sir I am waiting for your response

zhongjiajie commented 11 months ago

I approval the CI run

HarshitNagpal29 commented 11 months ago

Other LGTM, but it seems you forget to format the code by too -e auto-lint

Sorry for the inconvenience i will try to do it now.

zhongjiajie commented 11 months ago

you can run the command and then append one more commit this this PR

HarshitNagpal29 commented 10 months ago

@zhongjiajie sir i don't know why but i am not able to setup and run tox command on my local machine, I have tried a lot really sorry for that but still I have done the formatting, linting, styling and all using black and ruff commands manually, please check if it still needs improvement, again sorry for inconvenience.

zhongjiajie commented 10 months ago

I approval CI run. what OS do you use in your local machine?

HarshitNagpal29 commented 10 months ago

@zhongjiajie sir i use mac os. Also sir what to do with these CI run which is failing.

zhongjiajie commented 10 months ago

@zhongjiajie sir i use mac os. Also sir what to do with these CI run which is failing.

tax should work with macOS, and you should format http.py files because log said

lint: 15588 W commands[0]> python -m black --check . [tox/tox_env/api.py:427]
[549](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:550)
would reformat /home/runner/work/dolphinscheduler-sdk-python/dolphinscheduler-sdk-python/src/pydolphinscheduler/tasks/http.py
[550](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:551)

[551](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:552)
Oh no! 💥 💔 💥
[552](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:553)
1 file would be reformatted, 157 files would be left unchanged.

zhongjiajie commented 10 months ago

do you install tox first then trying run command tox -e auto-lint? and how about the output when you run tox -l?

HarshitNagpal29 commented 10 months ago

@zhongjiajie sir i use mac os. Also sir what to do with these CI run which is failing.

tax should work with macOS, and you should format http.py files because log said

lint: 15588 W commands[0]> python -m black --check . [tox/tox_env/api.py:427]
[549](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:550)
would reformat /home/runner/work/dolphinscheduler-sdk-python/dolphinscheduler-sdk-python/src/pydolphinscheduler/tasks/http.py
[550](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:551)

[551](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:552)
Oh no! 💥 💔 💥
[552](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7435014085/job/20230557494?pr=130#step:5:553)
1 file would be reformatted, 157 files would be left unchanged.

@zhongjiajie sir I made changes in http.py file using tox , I used different machine with linux as OS , in my mac machine even after installing tox when I was inputting "tox -l" command, it was showing me only version of python that is "312" no file like tox.ini and all, maybe it was some kind of a glitch.

zhongjiajie commented 10 months ago

maybe you can remove the project and try clone from the GitHub again after this PR merged

HarshitNagpal29 commented 10 months ago

@zhongjiajie ok I will try that method, but do you need any more changes in this pull request

zhongjiajie commented 10 months ago

lint still no pass, maybe you can run command python -m ruff check --fix --unsafe-fixes . to fix it

lint: 16342 W commands[1]> python -m ruff check . [tox/tox_env/api.py:427]
[553](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7438828861/job/20249516885?pr=130#step:5:554)
tests/tasks/test_http.py:167:13: F841 Local variable `http` is assigned to but never used
[554](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7438828861/job/20249516885?pr=130#step:5:555)
Found 1 error.
[555](https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7438828861/job/20249516885?pr=130#step:5:556)
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).

HarshitNagpal29 commented 10 months ago

@zhongjiajie sir please check it now, i fixed that linting issue and in my machine it is showing me everything fine

HarshitNagpal29 commented 10 months ago

@zhongjiajie sir please provide approval now

zhongjiajie commented 10 months ago

test error for case test_http_params_deprecation_warning with link https://github.com/apache/dolphinscheduler-sdk-python/actions/runs/7445619872/job/20284481402?pr=130

codecov-commenter commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.89%. Comparing base (f16599f) to head (3a22ec0). Report is 9 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #130 +/- ## ========================================== + Coverage 90.86% 90.89% +0.02% ========================================== Files 64 64 Lines 2354 2361 +7 ========================================== + Hits 2139 2146 +7 Misses 215 215 ``` | [Flag](https://app.codecov.io/gh/apache/dolphinscheduler-sdk-python/pull/130/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/apache/dolphinscheduler-sdk-python/pull/130/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.89% <100.00%> (+0.02%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zhongjiajie commented 10 months ago

Hi @HarshitNagpal29 I add and fix some tests in 3a22ec0

for the test_http_params we need to mock the return of function gen_code_and_version which is called with dolphinscheduler code in apache/dolphinscheduler

@patch(
    "pydolphinscheduler.core.task.Task.gen_code_and_version",
    return_value=(123, 1),
)

and for case test_http_params_deprecation_warning, w[-1].message is detail of warnings message, which depend on your warning message in the class Http

zhongjiajie commented 10 months ago

Now all CI are green, merging 🎉 thanks for your contribution and looking forward your next contribute

HarshitNagpal29 commented 10 months ago

@zhongjiajie sir i am really grateful that i was able to work on this issue with you and learn lot of new things, i am looking forward to contribute more , can you please suggest some good first issues which i can work upon or maybe some documentation changes in this project or some other.

zhongjiajie commented 10 months ago

Hi @HarshitNagpal29 here are two of issue maybe good and easy to fix

Docs changed only, you can learn how our docs build and render https://github.com/apache/dolphinscheduler/issues/8955
Code change, which you need to add utils class to detect and setValue and auto add out-put parameter for our code https://github.com/apache/dolphinscheduler-sdk-python/issues/61

apache / dolphinscheduler-sdk-python