-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactor suggestion: Migrating package from {httr}
to {httr2}
#13
Comments
Hi @elipousson I'd like to keep Thanks for the offer and for bringing |
Awesome. Thanks for being open to the idea. I'll dig into the dev branch and, if it seems like it may make sense to break this issue up into multiple pull requests, I'll be sure to check with you in this discussion before going too far in one direction or another. |
I got a bit sucked into this project last night and for a little too much time today as well. A couple of questions/suggestions that I'd like to implement:
I have everything working with httr2 except insert_records and update_records but I think I'll get those done by Friday. The changes may be a little more extensive than expected but, other than those two suggested changes, all the changes are wholly compatible with the way everything works right now. I also added some tests with testthat before starting so I could make sure everything stayed consistent. |
I would prefer keeping I'm open to the idea of deprecating batch size, pending a couple of clarifying questions:
Thanks for your work on this! When you get to the point of a pull request, I'd also appreciate if you could follow up here with a quick description of your testing process. I haven't yet come up with a clever way to unit/system test this code (if you've got any ideas, I'm all ears) |
Got it. I'll keep the For Your existing code for handling batches with vectorized versions of the standard functions made a lot of sense but I'm making some changes to batches size 10 or smaller just go through the regular version. I'm still working my way through the update records and insert records functions but the delete_records code is a good example of where I'm trying to get to for the others. The airtable_request.R file now has all of the main utility functions, esp. For testing, I have saved environmental variables with the base ID, table ID, and view ID for a test Airtable base and a helper function to skip tests if no PAT or API key is available. httr2 actually has functions for managing secrets specifically to support testing but I may also add tests built with httptest2. FWIW, I think this may look like an opinionated pull request so I really hope it doesn't feel like I'm stepping on your toes when you see it. Your code makes a ton of sense but, knowing that I have some ideas for using rairtable in some of my own packages and workflows, I'm trying to build in some extra flexibility into the helper functions in anticipation of those needs. There shouldn't be any breaking changes so hopefully it is all just a helpful evolution of the package you already built. |
I now have insert_records working (httr2 allowed some simplification of the pre-processing for the data to create new records)! airtable_base is also updated. The last function to go is update_records. You can see the new code for insert_records and in the create_records.R file. I also will need to re-implement the progress bar and parallel parameter option but hope to open a draft pull request before that point so you can review all the major changes. |
I'm excited to see a new R package for working with Airtable and I'm definitely interested in contributing! I added some Airtable data access functions to my own getdata package but struggled to get the offset working.
I was wondering if you may be open for a pull request to migrate the package from using
{httr}
to use{httr2}
. I've migrated a few different API packages from{httr}
or{jsonlite}
to{httr2}
over the past year or two and I've found that using{httr2}
makes it much easier to add new features and handle rate limiting and authentication. If you're open to it, I'd be happy to do a first pass on migrating the rairtable package over to{httr2}
.The text was updated successfully, but these errors were encountered: