Added method to GoogleBigQuery to copy between projects #1111
Conversation
Charlie-Kramer
force-pushed
the
main
branch
2 times, most recently
from
04d624c to
ef0b416
Compare
|
@Charlie-Kramer you seem to have accidentally added a bin folder with a bunch of files within - can you update the PR to remove them? Also that pyvenv.cfg top level file. Let me know if you need help |
|
@Charlie-Kramer overall this looks good, only two issues. First the extra files that got included in this PR that will need to be removed, and second it would be good to have a test for this new method. In particular, a test that tests the logic around missing datasets/tables/etc would be good. |
|
Hey thanks! I added the tests and cleaned up the formatting problems so I think it's ready for review. (Sorry prob should have put that in the message) |
|
I think the issue is that the files were added before you put them in the .gitignore, so they're still there in the PR - they have to be manually removed. Adding them to the gitignore prevents this from happening in the future but does not fix the currently added files. |
ah, I removed a bunch of them using git rm but there are others I missed. I did git rm bin -r, I hope this does the trick? |
|
When you did git rm, did you commit the changes? I don't see any new commits from you. |
|
Looks like it worked! |
pyvenv.cfg
Outdated
| @@ -0,0 +1,3 @@ | |||
| home = /Users/charleskramer/opt/anaconda3/bin | |||
There was a problem hiding this comment.
looks like you missed one last file that should be gitignored/removed
There was a problem hiding this comment.
ah let me get that one now. fixed the f string thing too.
| @@ -0,0 +1,116 @@ | |||
| # testing bigquery copy method of google class | |||
There was a problem hiding this comment.
So I see why you put this in test_google but bigquery code is actually tested under test_databases. It's an unfortunate bit of organization since the bigquery code is not in the databases folder (I'm not entirely sure it makes sense to have a databases folder at all, but that's a decision that predates me).
Anyway, can you move this to the existing test file here? I'm not sure if there's anything in the existing test file you'd want to reuse to make your tests easier to read but otherwise these tests look fine.
Apologies again for the confusing organization.
|
You bet! Thanks for your patience, I’m kinda making a mess here.
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Shauna Gordon-McKeon ***@***.***>
Sent: Monday, December 23, 2024 1:21:24 PM
To: move-coop/parsons ***@***.***>
Cc: Kramer, Charles ***@***.***>; Mention ***@***.***>
Subject: Re: [move-coop/parsons] Added method to GoogleBigQuery to copy between projects (PR #1111)
@shaunagm commented on this pull request.
________________________________
In test/test_google/test_bigquery_copy.py<#1111 (comment)>:
@@ -0,0 +1,116 @@
+# testing bigquery copy method of google class
So I see why you put this in test_google but bigquery code is actually tested under test_databases. It's an unfortunate bit of organization since the bigquery code is not in the databases folder (I'm not entirely sure it makes sense to have a databases folder at all, but that's a decision that predates me).
Anyway, can you move this to the existing test file here<https://github.com/move-coop/parsons/blob/main/test/test_databases/test_bigquery.py>? I'm not sure if there's anything in the existing test file you'd want to reuse to make your tests easier to read but otherwise these tests look fine.
Apologies again for the confusing organization.
—
Reply to this email directly, view it on GitHub<#1111 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A2VZMA4LBC3WIGUDG4MGNDT2HBICJAVCNFSM6AAAAABL6EV4ASVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRQHE4TONRZGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
You're doing great! It's Parsons that's messy :) Can you move the bigquery copy test into the test_bigquery.py file? You can keep it as a separate test class although you'll need to rename your class so you don't overwrite the existing class (both are named |
|
Sure! But then it won’t run automatically yes? (Maybe that’s the idea?)
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Shauna Gordon-McKeon ***@***.***>
Sent: Monday, December 23, 2024 1:41:43 PM
To: move-coop/parsons ***@***.***>
Cc: Kramer, Charles ***@***.***>; Mention ***@***.***>
Subject: Re: [move-coop/parsons] Added method to GoogleBigQuery to copy between projects (PR #1111)
You're doing great! It's Parsons that's messy :)
Can you move the bigquery copy test into the test_bigquery.py<https://github.com/move-coop/parsons/blob/main/test/test_databases/test_bigquery.py> file? You can keep it as a separate test class although you'll need to rename your class so you don't overwrite the existing class (both are named TestGoogleBigQuery. TestGoogleBigQueryProjectCopy or similar would be perfectly fine
—
Reply to this email directly, view it on GitHub<#1111 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A2VZMA4Q26BARCUJ5QDFJGD2HBKOPAVCNFSM6AAAAABL6EV4ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRQGE2TANRYG4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
As long as it's in a file that starts with `test_*` and is a class that
inherits from `TestCase` and as long as the individual methods start with
`test_*` it will run automatically. You can have multiple test classes in
the same test file and they'll all run as long as they each individually
satisfy the unittest syntax rules. And as long as they don't have the same
names (if they have the same names, whatever comes last will overwrite the
earlier versions)
…On Mon, Dec 23, 2024 at 1:46 PM Charlie Kramer ***@***.***> wrote:
Sure! But then it won’t run automatically yes? (Maybe that’s the idea?)
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Shauna Gordon-McKeon ***@***.***>
Sent: Monday, December 23, 2024 1:41:43 PM
To: move-coop/parsons ***@***.***>
Cc: Kramer, Charles ***@***.***>; Mention ***@***.***>
Subject: Re: [move-coop/parsons] Added method to GoogleBigQuery to copy
between projects (PR #1111)
You're doing great! It's Parsons that's messy :)
Can you move the bigquery copy test into the test_bigquery.py<
https://github.com/move-coop/parsons/blob/main/test/test_databases/test_bigquery.py>
file? You can keep it as a separate test class although you'll need to
rename your class so you don't overwrite the existing class (both are named
TestGoogleBigQuery. TestGoogleBigQueryProjectCopy or similar would be
perfectly fine
—
Reply to this email directly, view it on GitHub<
#1111 (comment)>,
or unsubscribe<
https://github.com/notifications/unsubscribe-auth/A2VZMA4Q26BARCUJ5QDFJGD2HBKOPAVCNFSM6AAAAABL6EV4ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRQGE2TANRYG4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
—
Reply to this email directly, view it on GitHub
<#1111 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI75YRX2EJ4UHJFWVXJHYD2HBK7TAVCNFSM6AAAAABL6EV4ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRQGE2TKMBTGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
can we do a pair programming at some point? I reformatted using ruff locally but now it fails the ruff checks on github. I tried going back to a previous commit but that didn't work. |
|
Sure - you have my cal.com link in Slack DMs, right? Feel free to sign up
for whatever's next available (I'm off for holidays but I should have
normal times again in the new year)
…On Mon, Dec 23, 2024 at 3:08 PM Charlie Kramer ***@***.***> wrote:
can we do a pair programming at some point? I reformatted using ruff
locally but now it fails the ruff checks on github. I tried going back to a
previous commit but that didn't work.
—
Reply to this email directly, view it on GitHub
<#1111 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI75YQ4T47RKZVUYSFLUSD2HBUSBAVCNFSM6AAAAABL6EV4ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRQGI2DAOBYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Ah I got it to pass all the tests! I did something with ruff that wasn't entirely sensible I think.
Anyway my main question is how to run the tests locally but I can post on the slack about this.
Happy Holidays! Hope you're having a good one.
________________________________
From: Shauna Gordon-McKeon ***@***.***>
Sent: Monday, December 23, 2024 4:21 PM
To: move-coop/parsons ***@***.***>
Cc: Kramer, Charles ***@***.***>; Mention ***@***.***>
Subject: Re: [move-coop/parsons] Added method to GoogleBigQuery to copy between projects (PR #1111)
Sure - you have my cal.com link in Slack DMs, right? Feel free to sign up
for whatever's next available (I'm off for holidays but I should have
normal times again in the new year)
On Mon, Dec 23, 2024 at 3:08 PM Charlie Kramer ***@***.***> wrote:
can we do a pair programming at some point? I reformatted using ruff
locally but now it fails the ruff checks on github. I tried going back to a
previous commit but that didn't work.
—
Reply to this email directly, view it on GitHub
<#1111 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI75YQ4T47RKZVUYSFLUSD2HBUSBAVCNFSM6AAAAABL6EV4ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRQGI2DAOBYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
—
Reply to this email directly, view it on GitHub<#1111 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A2VZMAYZGSN2TMRVXACVUGL2HB5GDAVCNFSM6AAAAABL6EV4ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRQGMYTAMJUGA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
No description provided.