search

ropensci / rppo

R package for accessing PPO data store
https://docs.ropensci.org/rppo
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

Remove hardcoded `source` and add as argument instead #20

Closed Peter9192 closed 1 year ago

Peter9192 commented 1 year ago

This PR removes the hardcoded source argument as suggested in #18. Instead, it adds "source" as an additional argument. I'm not sure what you prefer for the default value of this keyword:

  1. Default to "USA-NPN": this would mean the behaviour doesn't change, but it is quite inconsistent with the rest of the options. Users would manually have to set source = NULL to not filter for any specific source
  2. Default to NULL: more consistent with other keywords, more intuitive. But this changes the behaviour, such that calls to ppo_data that previously used to return only NPN data may now include data from other sources as well.

Currently, I picked option 2, which would be my personal preference. But I could change it back to option 1 if you think option 2 excessively impacts existing users.

This PR also fixes a typo in the unlink comment for the keepData option, which led to buggy behaviour as reported here.

closes #18

Peter9192 commented 1 year ago

@jdeck88 can you approve the CI workflow?

jdeck88 commented 1 year ago

I think option 2 is fine....