search

ceff-tech / ffc_api_client

An R client for the online Functional Flows Calculator API
https://ceff-tech.github.io/ffc_api_client
9 stars 3 forks source link

add nhdplusTools functionality w get comids for lat lon and usgs ids #13

Closed ryanpeek closed 4 years ago

ryanpeek commented 4 years ago

check this out and see if it works. I think you may need to change your NAMESPACE as well (export(get_comid_for_lon_lat) instead of long_lat) since I didn't want to mess with too much of the package bits that get autogenerated. But I've tested this on my end and it seems to work.

nickrsan commented 4 years ago

Hey Ryan - looks good to me overall. My only concern is that as gages get added or removed, the package gets out of date unless we manually update the gage list. This isn't a big concern, but part of me wonders if it'd be worth just pulling the list each time the code runs - if it takes 5 or 10 seconds I'm not too worried about it personally. We could also do this by refactoring the code into a class so it pulls the list into a class variable so it only does it once, but that's still a big change - but I'd been thinking about making the main FFC communication code a class anyway.

Regardless, I don't see anything preventing a merge right now. I haven't run it on my machine, but I'm fine merging it at this point based on it working on yours and then if we run into issues, we'll resolve them with the knowledge that we'd like to go down the route of the code you've written for this anyway.

Last thing - I'm curious what issues you were having with the example roxygen comments - your commits made it sound like they weren't working?

Thanks for this - Should be great to have this with the new library.

ryanpeek commented 4 years ago

All good points Nick, and yes we probably should just set it up to pull a gage list each time, I'm fine with that. Thanks for making this happen, and apologies for the slog of commit near the end. I was trying run a package check, and it was failing with the Example (not sure why). So I was trying to remove that bit, but that didn't seem to help either. Could be I just don't know what I'm doing and its obvious for folks familiar with building/fixing packages. SO...not sure but can troubleshoot later. Ultimately it seems everything is working, so we can iron out these bits later. Thanks for your patience with my code/git madness. :)

ryanpeek commented 4 years ago

Thanks Nick! sorry for the chaos. Also I forgot to add a semi important bit to the NAMESPACE file. Can you add export(get_comid_for_usgs_gage)? Then folks can query for a given gage pretty easily....forgot to export the function and probably easier for you to make change than for me to make a whole new PR, but happy to do so if you'd like.

I'll play around with the example issue on my fork and see if I can figure out why it's breaking. I'm less familiar with it, and not sure if it has to do with things living outside the package folder but still in the git repo. May not matter....but not sure.

THanks!

R

On Fri, Dec 20, 2019 at 7:20 PM Nick Santos notifications@github.com wrote:

Hey Ryan - looks good to me overall. My only concern is that as gages get added or removed, the package gets out of date unless we manually update the gage list. This isn't a big concern, but part of me wonders if it'd be worth just pulling the list each time the code runs - if it takes 5 or 10 seconds I'm not too worried about it personally. We could also do this by refactoring the code into a class so it pulls the list into a class variable so it only does it once, but that's still a big change - but I'd been thinking about making the main FFC communication code a class anyway.

Regardless, I don't see anything preventing a merge right now. I haven't run it on my machine, but I'm fine merging it at this point based on it working on yours and then if we run into issues, we'll resolve them with the knowledge that we'd like to go down the route of the code you've written for this anyway.

Last thing - I'm curious what issues you were having with the example roxygen comments - your commits made it sound like they weren't working?

Thanks for this - Should be great to have this with the new library.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ceff-tech/ffc_api_client/pull/13?email_source=notifications&email_token=ABMX3MHEX2HYWTQI75FMD7TQZWDOLA5CNFSM4J6ETWVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHOT5VA#issuecomment-568147668, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMX3MENPLTTP2BWY2V5XILQZWDOLANCNFSM4J6ETWVA .

--

"When we try to pick out anything by itself, we find it hitched to everything else in the universe."John Muir (My First Summer in the Sierra, 1911) ----------------------------------------------------- Ryan Peek, PhD (he/him) Aquatic Ecologist, Post-Doctoral Researcher Center for Watershed Sciences, UC Davis ryanpeek.github.io http://ryanpeek.github.io @riverpeek 530.754.5351

-----------------------------------------------------