Skip to content
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

Adding geocode_csv() and geocode_csv_reverse() function #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

feddelegrand7
Copy link

Hi I've added the following which I think may complement well the current features of the package:

  1. Adding the geocode_csv() function
  2. Adding the geocode_csv_reverse() function
  3. Both function are documented (Rd files added)
  4. Both functions are documented within the vignetter Rmd

Best regards.

1. Adding the geocode_csv() function
2. Adding the geocode_csv_reverse() function
3. Both function are documented (Rd files added)
4. Both functions are documented within the vignetter Rmd
Copy link
Owner

@joelgombin joelgombin left a comment

Choose a reason for hiding this comment

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

Hi @feddelegrand7 ! Thanks for your PR!

In the present state of {banR} dealing with the CSV files is done under the hood as we were considering the use case where one is working with a tibble/dataframe. But it can be useful to be able to deal with CSV files directly, so I feel the PR is welcome.

I'm not completely comfortable with the API of these functions though. In my opinion, having something like:

geocode_csv(file = "search.csv", 
            columns = "postcode", 
            columns = "adresse")

is not very idiomatic in R.
Maybe it could be something like:

geocode_csv(file = "search.csv", 
            columns = c("postcode", "adresse"))

It would make the function code a bit more convoluted but I feel this syntax would be more natural to R users. What do you think @pachevalier ?

@feddelegrand7
Copy link
Author

feddelegrand7 commented Nov 7, 2020

Hi @joelgombin thanks for your feedback. You're absolutely right, columns = c("postcode", "adresse") would be way more efficient for the end user and is R friendly however to be honest I've tried to pass a vector to the body paramater of the POST() function (and a list also) but it didn't work, that's why I used the .... Any solution for that ?

@pachevalier
Copy link
Collaborator

What's the diff with tbl_geocode?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants