Skip to content

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

Merged
merged 78 commits into from
May 20, 2020
Merged

Conversation

sofisl
Copy link
Contributor

@sofisl sofisl commented Apr 7, 2020

No description provided.

@sofisl sofisl requested a review from leahecole April 7, 2020 16:54
@sofisl sofisl requested a review from a team as a code owner April 7, 2020 16:54
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 7, 2020
Copy link
Contributor

@tmatsuo tmatsuo left a 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?

@leahecole leahecole self-assigned this Apr 7, 2020
@sofisl sofisl requested review from leahecole and tmatsuo April 7, 2020 22:33
@sofisl
Copy link
Contributor Author

sofisl commented Apr 7, 2020

@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.

Copy link
Contributor

@tmatsuo tmatsuo left a 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.

Copy link
Contributor

@davidcavazos davidcavazos left a 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
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor

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?

@sofisl sofisl requested a review from leahecole May 19, 2020 16:26
Copy link
Collaborator

@leahecole leahecole left a 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!

@leahecole leahecole added the automerge Merge the pull request once unit tests and other checks pass. label May 20, 2020
@leahecole leahecole merged commit 6e6b7e2 into master May 20, 2020
@leahecole leahecole deleted the dagTesting branch May 20, 2020 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants