Check in Cloud Job Discovery client code samples for python by xinyunh0929 · Pull Request #1546 · GoogleCloudPlatform/python-docs-samples

xinyunh0929 · 2018-06-29T00:23:19Z

CJD got Public beta launch so we would like to have our samples checked in as other projects.

xinyunh0929 · 2018-06-29T00:25:00Z

Still working on the samples. But I would like to know how to run local tests for my samples.
I saw there are "_test" files under other project, but it seems I could not run it directly.

xinyunh0929 · 2018-07-03T17:17:40Z

I am working on the tests to those samples, however, I don't know how to run those '_test' files. Is there any guide?

googlebot · 2018-07-16T22:46:05Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

michaelawyu · 2018-07-16T23:18:57Z

+from googleapiclient.discovery import build
+
+client_service = build('jobs', 'v2')
+


You can remove these empty lines between code and region tag.

I use pyformat -i -s 4 --recursive . to format my code. And it seems this is recommended.
Should I manually modify those files? Or it should be fine?

michaelawyu · 2018-07-16T23:25:18Z

+
+import time
+
+# [START instantiate]


For the region tag it might be better if you add the product name (job, maybe) as a prefix, i.e. use job_setup instead of setup.

This is for tag consistence among different languages since TW prefer them in the same name. Do you think it is necessary to modify the tag?

michaelawyu · 2018-07-16T23:36:07Z

+# [END instantiate]
+
+
+# [START basic_company]


Maybe it's not necessary to declare a function here with a specific region tag. How about just specifying a company variable?

company = { 'display_name': 'Google', 'distributor_company_id': distributor_company_id, 'hq_location': '1600 Amphitheatre Parkway Mountain View, CA 94043' }

You can use a fixed ID here as well I think.

I could not use a fixed ID since creating an entity with same ID would lead to the failure of tests

No. This function is required from doc site and I couldn't use the fixed ID due to our system.

michaelawyu · 2018-07-16T23:37:15Z

+        complete.companyName = company_name
+
+    results = complete.execute()
+    print('==========\n%s\n==========' % results)


The padding part ========== may not be necessary.

Just want to make the output clear for customer. I could remove it if you think it is the better.

michaelawyu · 2018-07-16T23:38:40Z

+            body=company_to_be_created).execute()
+        print('==========\nCompany created: %s\n==========' % company_created)
+        return company_created
+    except HttpError as e:


Use the base error class (googleapiclient.errors.Error) would be better I think. Or you could just drop the try/except block and the let the error raise.

michaelawyu · 2018-07-16T23:45:10Z

+
+
+# [START basic_job]
+def generate_job_with_required_fields(company_name):


I feel that this can be a simple job variable as well.

Just to distinguish this function from other generate job function such as generate_job_with_custom_attribute

michaelawyu · 2018-07-16T23:48:20Z

+
+# [START batch_job_create]
+def batch_job_create(client_service, company_name):
+    import base_job_sample


I understand your concern here; but for clarity I do feel that this part can be replaced with two simple job variables so that developers do not need to read another file to understand what's going on behind the scenes.

It could be changed later since it request modifying the samples in all languages.

michaelawyu · 2018-07-17T00:02:56Z

+        job_to_be_updated = jobs_to_be_updated[index]
+        job_to_be_updated.update({'job_title': 'Engineer in Mountain View'})
+        request = {'job': job_to_be_updated}
+        if index % 2 == 0:


This part might be a little bit confusing I think. If we are showing the batch updating method do we really need to showcase updating only half of the jobs and leaving the other half alone? We can just update all of them.

They want to have the updateRequest one with the field mask feature, one is not.

michaelawyu · 2018-07-17T00:04:12Z

+            pass
+
+    batch = client_service.new_batch_http_request()
+    for index in range(0, len(jobs_to_be_updated)):


Side note: if you switch to updating all jobs instead, this part can become for job in jobs, which is easier to understand and more idiomatic for Python devs.

I know. However, as mentioned above, they want half jobs updated using field mask feature, and half not.

michaelawyu · 2018-07-17T00:04:37Z

+            pass
+
+    batch = client_service.new_batch_http_request()
+    for index in range(0, len(jobs_to_be_deleted)):


The index is not necessary though; how about for job in jobs?

michaelawyu · 2018-07-17T00:09:54Z

@@ -0,0 +1,36 @@
+# This file must be used with "source bin/activate.csh" *from csh*.
