Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add explanation for arguments passing #1746

Merged

Conversation

magicpieh28
Copy link
Contributor

@magicpieh28 magicpieh28 commented Jun 8, 2022

I added explanation about how to run the class or function that needs arguments when using py>.

Currently, there is no explanation in the digdag documents, but actually we can pass arguments using _export. So I thought the explanation needed.

@szyn szyn added the document label Jun 9, 2022
@yoyama
Copy link
Contributor

yoyama commented Jun 24, 2022

@magicpieh28 Thank you for your contribution.

Based on the example https://github.com/treasure-data/digdag/blob/master/examples/python_args.dig
It describe another way to pass argument.
So if we add the description about the way to pass argument in doc, it might be better to describe both way.

@yoyama
Copy link
Contributor

yoyama commented Jul 8, 2022

@magicpieh28 Thank you for your update.
I hope add description about the way to pass argument as task parameters.

- one more way how to pass arguments
- data types of passed arguments
@magicpieh28 magicpieh28 requested a review from yoyama July 12, 2022 08:24
@yoyama yoyama added this to the v0.10.5 milestone Jul 27, 2022
print(f"{required1}: {required2}")
```

Finally, you can pass combination of class and mehtod arguments to Python script as the following:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is a bit strange.
IIUC, we can't use same argument name between class initializer and method argument for it.
It might be better to add a warning about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my poor English. Just to be clear, what I tried to explain is in digdag, indeed we can pass both(class and method) arguments at once.
So, on-premise that the above behavior is correct what I understood is I just need to add a warning like ‘But you can not pass combination with the same name.’ instead of modifying the example.
Is this correct? @yoyama

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@magicpieh28 Sorry for my comment is not clear.
Yes, your description and example are correct. But I feel a bit strange on this specification on py>.
Especially, if both initializer of Class and the method have same parameter name, we can't set them separately. (If I understand correctly).
So I propose add description to warn about it as you describe.
Anyway thank you for your contribution.

…fferent names when we pass those through digdag
@magicpieh28 magicpieh28 requested a review from yoyama August 9, 2022 01:40
@magicpieh28
Copy link
Contributor Author

@yoyama
Please review my modification. Thank you.


This example assume following Python script:

```python
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example does not work by copy and paste it.
Don't we need to import Union as follows ?

from typing import Union

class MyWorkflow(object):
...

@yoyama
Copy link
Contributor

yoyama commented Sep 26, 2022

@magicpieh28 Sorry for my late reply. I left a comment. Could you take a look it ?

Copy link
Contributor

@yoyama yoyama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for your contribution!

@yoyama yoyama merged commit 2813e02 into treasure-data:master Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants