Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

GoogleDisplayVideo360RecordSDFAdvertiserOperator CSV parse fix #20

Merged
ceoloide merged 2 commits into from
Mar 2, 2020
Merged

GoogleDisplayVideo360RecordSDFAdvertiserOperator CSV parse fix #20

ceoloide merged 2 commits into from
Mar 2, 2020

Conversation

@Jimbol4
Copy link
Contributor

@Jimbol4 Jimbol4 commented Feb 28, 2020

  • Uses csv.reader to parse CSV instead of str.split(',')
  • Fixes bug encountered where CSV fields containing commas would cause invalid data to be saved to the Airflow variable (breaking the DAGs that use this variable).

@googlebot
Copy link

googlebot commented Feb 28, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@Jimbol4
Copy link
Contributor Author

Jimbol4 commented Feb 28, 2020

@googlebot I signed it!

ceoloide
ceoloide suggested changes Feb 28, 2020
View reviewed changes
Copy link
Contributor

@ceoloide ceoloide left a comment

Choose a reason for hiding this comment

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

Suggesting a switch to csv.DictReader, else the code looks good to be merged.

@ceoloide
Copy link
Contributor

ceoloide commented Feb 28, 2020

@googlebot can you recheck this PR?

@googlebot
Copy link

googlebot commented Mar 2, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

ceoloide
ceoloide approved these changes Mar 2, 2020
View reviewed changes
Copy link
Contributor

@ceoloide ceoloide left a comment

Choose a reason for hiding this comment

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

All changes look good, thank you for your contributions!

@ceoloide ceoloide merged commit 8431ac6 into google:master Mar 2, 2020
ceoloide added a commit that referenced this pull request Mar 2, 2020
@ceoloide
Squashed commit of the following:
7ba2c4a 
commit 8431ac6
Merge: 5cbc67b 9257165
Author: Marco Massarelli <60667061+ceoloide@users.noreply.github.com>
Date:   Mon Mar 2 09:38:30 2020 -0500

    Merge pull request #20 from Jimbol4/csv-parse-fix

    GoogleDisplayVideo360RecordSDFAdvertiserOperator CSV parse fix

commit 9257165
Author: Jimbol4 <jimbola@gmail.com>
Date:   Mon Mar 2 11:55:38 2020 +0000

    Change parsing method to use DictReader

commit 9e1d87f
Author: Jimbol4 <jimbola@gmail.com>
Date:   Fri Feb 28 11:19:51 2020 +0000

    Parse CSV with csv reader instead of string comma split

commit 5cbc67b
Merge: b42be20 006853f
Author: Marco Massarelli <mmassarelli@google.com>
Date:   Thu Feb 6 17:55:36 2020 -0500

    Merge remote-tracking branch 'gob/master'

commit b42be20
Merge: 864c444 cac9932
Author: Kingsley <kingsleykelly@google.com>
Date:   Tue Jan 14 10:02:54 2020 +0000

    Merge pull request #16 from google/sdf_v5

    Adding support for SDF version 5

commit cac9932
Author: Tom Oczos <41787263+oczos@users.noreply.github.com>
Date:   Fri Jan 10 11:09:52 2020 +0000

    Adding support for SDF version 5

    Added table schemas to support SDF version 5 output

commit 864c444
Merge: 841d30d b92dad0
Author: Tom Oczos <41787263+oczos@users.noreply.github.com>
Date:   Mon Dec 9 13:56:44 2019 +0000

    Merge pull request #15 from google/kw-args

    Update Readme and update Entity Read Files for new Hook

commit b92dad0
Author: Kingsley Kelly <kingsleykelly@google.com>
Date:   Fri Nov 8 10:37:13 2019 +0000

    Update README

    Change-Id: Ia0e95320fe2d154b2aa4dcf1938ac2ea9bc41370

commit 70f7f5b
Author: Kingsley Kelly <kingsleykelly@google.com>
Date:   Wed Nov 6 11:31:05 2019 +0000

    Update readme

    Change-Id: Iebc1af9d3e65030abd3691a03bfb51bc467ba645

commit 841d30d
Merge: 7a2a370 43a0849
Author: Kingsley Kelly <kingsleykelly@google.com>
Date:   Mon Nov 4 18:02:35 2019 +0000

    Merge branch 'master' of https://github.com/google/orchestra

    Change-Id: Ib6c6e42c395b46ec8132ef28f627aa1a7762a465

commit 7a2a370
Author: Kingsley Kelly <kingsleykelly@google.com>
Date:   Mon Nov 4 18:00:03 2019 +0000

    Remove table of contents

    Change-Id: I0cb6f566f81ff0f83fa05f71aa8b2876276e10d9

commit 43a0849
Merge: 2f73de9 2bf53c1
Author: Tom Oczos <41787263+oczos@users.noreply.github.com>
Date:   Mon Nov 4 17:53:26 2019 +0000

    Merge pull request #13 from google/Orchestra-2.0

    Orchestra 2.0

Merging GitHub pull request #20 changes, while squashing them trying to
resolve missing Change-Id issue in Gerrit.

Change-Id: I47fb0f66b618d88438c90d61d3bccb0403869148
ceoloide added a commit that referenced this pull request Mar 2, 2020
@ceoloide
Merge branch 'master' of github.com:google/orchestra
4803530 
Merging with pull request #20.

Change-Id: Ie2f50659f4c1ebd45a01998d2f647e198e0df4b4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ceoloide ceoloide ceoloide approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Jimbol4 @googlebot @ceoloide