cynkra / sdtools

tools to work with swissdata sets
1 stars 0 forks source link

clean up sdtools #5

Closed christophsax closed 5 years ago

christophsax commented 5 years ago

The goal is to open up sdtools and make it publicly available, at least on GitHub.

How familiar are you with package building?

sdtools should be cleaned up. E.g., we should:

If it is too new for you, I can do it myself.

karoliskoncevicius commented 5 years ago

All good, I have 2 packages on CRAN, so familiar with their checks.

A few things to clear up would be nice:

Right now read_swissdata() and read_swissdata_json() are in the same file, but read_swissdata_s3() is separated for some reason. Will other read functions be deprecated?

christophsax commented 5 years ago

Let's rename read_swissdata to read_swissdata_yaml, so we will have all this three functions.

In the long run, we may switch to JSON, but that may take a while.

karoliskoncevicius commented 5 years ago

Two NOTEs remain as described in #12

christophsax commented 5 years ago

Looks great at a first glance, thanks a lot.

Will check and adjust details, as we use the functions. But this seems to be a very good start.

karoliskoncevicius commented 5 years ago

@christophsax

R CMD check --as-cran would pass, except for those 2 notes. So if we want to close this issue, we need to:

  1. Decide what to do with link to "bug report" - we can remove the link to this private repo for now.
  2. Decide what to do about the non-standard evaluation of "."' and "value" in dplyr, as they are passed without enclosing them as strings - R check looks for variables of that name, fails to find them, and issue notes.

One way I found in SO is to make empty objects of the same name, i.e.:

. <- value <- NULL

But a bit of a hack. Since you certainly know more about tidyverse than I do - maybe you know of a standard way to deal with this?

karoliskoncevicius commented 5 years ago

Note: after removing pipes, there is no more ".".

The "value" is only used in a few places when testing the data. I can modify those two lines to use base-R subsetting instead and that would resolve this note.

But will wait for your comments and input first.

christophsax commented 5 years ago

‚value <- NULL‘ is fine. And no need to care about the URLs, we can address that later.

Important is that we only have NOTEs, so I can set up Travis and ignore them there.

We can close this, otherwise.

karoliskoncevicius commented 5 years ago

Final pull req regarding this issue: #18

Closing for now.