-
Notifications
You must be signed in to change notification settings - Fork 6.6k
feat: add dataflow-composer tutorial code samples #3290
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
Conversation
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 reviewed until the file dataflowtemplateoperator_tutorial_test.py
.
I don't quite understand why you need lots of files after composerTutorial/bin/activate
. Can you add some explanations about this in the description?
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table_test.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table_test.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table_test.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table_test.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table_test.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table_test.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/dataflowtemplateoperator_tutorial.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table_test.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/create_dataset_and_table.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/dataflowtemplateoperator_tutorial.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/dataflowtemplateoperator_tutorial.py
Outdated
Show resolved
Hide resolved
composer/workflows/dataflow_template_operator_tutorial/dataflowtemplateoperator_tutorial.py
Outdated
Show resolved
Hide resolved
@tmatsuo and @leahecole, thank you both for your comments! This is my first time submitting Python code, so I really appreciate the in-depth review. |
…cs-samples into dagTesting
…cs-samples into dagTesting
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.
Personally I prefer main
+ argparse
, but I can live with the current form.
composer/workflows/dataflowtemplateoperator_create_dataset_and_table_test.py
Outdated
Show resolved
Hide resolved
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 really sorry to keep going back to the same issue. I just want to see what everyone thinks about it. Sofi and Leah, I know you've already changed this many times and it must be very annoying, so sorry about that. I'm happy to approve if everyone is happy with how it is.
|
||
from google.cloud import bigquery | ||
|
||
# TODO(developer): Replace with your values |
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.
@tmatsuo I'm personally not a big fan of requiring users to modify the source code to be able to run a sample. Would you be okay with getting these arguments directly from sys.argv
as positional arguments to avoid using argparse
?
Maybe something like this?
assert len(sys.argv) == 4, 'usage: ataflowtemplateoperator_create_dataset_and_table_helper.py PROJECT LOCATION DATASET_NAME'
project, location, dataset_name = sys.argv[1:]
Although argparse
might be clearer and would have better error reporting. What do you think?
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.
It's against our Code Snippet Style Guide: go/code-snippets-style#running-from-the-command-line
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.
@davidcavazos
Personally I have a slight preference on using sys.argv
, but I also understand the rationale of the style guide. The rationale of discouraging the use of sys.argv
or argparse
is value/maintenance cost ratio.
Our assumption: majority of users stay on the cloudsite docs and copy the code into their project to start. Providing a CLI only add values for small amount of users who actually git clone the sample repo.
There is certain amount of maintenance cost for providing a CLI. The current main is also not tested, but it's rather easy to maintain.
Does it make sense?
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.
LGTM when lint passes!
No description provided.