ropensci / essurvey

Download data from the European Social Survey
https://docs.ropensci.org/essurvey
Other
49 stars 9 forks source link

Distinguish download and import functionality as distinct functions #22

Closed leeper closed 6 years ago

leeper commented 6 years ago

The ess_rounds() function, and related functions, can both import and simply down a data file. As far as I can tell when importing, the data file is not saved to the local directory. It seems you may want to distinguish these by creating separate download_rounds() and import_rounds() functions since their behavior is quite different - the former is only used for a side effect.

This then has an impact on a feature of ess_rounds() that seems unnecessary for most users: namely, the ability to download different file formats. If the users is only importing into R, it shouldn't matter what file format they download so it might be safe to remove this option entirely from an import_rounds() function.

cimentadaj commented 6 years ago

This change makes a lot of sense and I think is the right way to approach this. In fact, the This change makes a lot of sense and I think is the right way to approach this. In fact, the implementation should be straight forward. However, I'm concerned about backward compatibility and the fact that I plan to change the name of the package to essurvey in the near future.

My main concern is having both the breaking compatibility change in the ess package and then changing the name of the package in a short period of time. I would want to give some time to users to communicate the API changes and let them know that the functions will be deprecated without also swamping them with a new package name (let's remember that the users of this package are not necessarily R users but people who come to R for specific things). I've thought a lot about this and I think this could be one solution:

I'm really hesitant on this step so I'd like to hear your comments. Perhaps it's not the best idea to apply these changes in the first place because the breaking changes are not extremely necessary.

I'd really like to hear your feedback on this particular point.

leeper commented 6 years ago

My advice would be to deprecate the current function using .Deprecated() and then remove it sometime in the future. As long as you're changing the package name, that seems like a fine time to deprecate (or even remove as users of the new package will likely be expecting API changes).

cimentadaj commented 6 years ago

Thanks @leeper ! I implemented the change in de3530413a32a6d86950bcdd23c22e7761d258f3 and 90ee16645cbd552a2b1c07f501590b3610422601. The package is now renamed and I'll submit to CRAN soon.