-
Notifications
You must be signed in to change notification settings - Fork 4
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
Initial package for getting and loading the data. #3
Conversation
@spkaluzny @TR4Android, @glennon, I've looked through all of it, and it looks good to me. It implements all of the usage I built in my push. I like the standardized naming process as well. I like having Along the lines of developing and package attribution, @spkaluzny has entered his author information. I can update author information for me and J. Hathaway. @TR4Android and @glennon do you want to provide your name and email so we can update the R description file? I would like to start working on the help functions. I'll document out the functions and provide a glossary to the variables in the help file. |
Sorry for the late reply, I just gave this a spin and this looks like an awesome starting point. The way the latest archive version is determined is a bit finicky currently, but that's the best we can currently do. As to the column names, this is something that should definitely be more standardized (@codemasta14 you can find a short glossary on our website, not all fields are contained in the archive though). Discussion on that can continue in issue #4 and I'll merge any needed code changes in a separate pull request. I love the added documentation, as I see this being an important aspect if we want people to actually use the package 😃. It would be nice if we could keep the vignettes up to date while we develop this, function references can be added at a later point, I guess. I'll try to get a pkgdown website up and running when the package is finished. @spkaluzny One minor nitpick: can we move everything up one folder (i.e. move the contents of the Thanks for taking the time to develop and review this! |
@TR4Android we can move the package code in my branch up one folder but that folder should be called |
On Wed, May 8, 2019 at 12:28 PM Cody Jorgensen ***@***.***> wrote:
@spkaluzny <https://github.com/spkaluzny> @TR4Android
<https://github.com/TR4Android>, @glennon <https://github.com/glennon>,
I've looked through all of it, and it looks good to me. It implements all
of the usage I built in my push. I like the standardized naming process as
well. I like having gt_ to start the functions. Didn't we discuss having
gt_load_data as an internal function and gt_read_data was available to
the user? Maybe we could have a wrapper function of the two that is
gt_data? I can start on that after we accept this push. Let's go ahead
and accept this pull request.
I wrote two functions, `gt_get_data` which is called by the user only once
and `gt_load_data` which the user calls in every session where she wants to
analyze the data. Because of CRAN rules, the default for `gt_get_data` is
to put the downloaded data under `tempdir()` but a message is printing say
that using `gt_path` will allow the data to persist. The default location
for `gt_get_data` is to load the data from `gt_path`. If we write a wrapper
function, `gt_data` to combine the two operations, the default location for
the data will have to be `tempdir()` and the data will not persist unless
the users sets a specific location overriding the default. I think this
behavior could lead to users calling `gt_data` with the default every time
which results in the data being downloaded in every session. But I could be
persuaded that this would not happen or is not an issue.
-Stephen
|
@spkaluzny Two things:
|
The archive that we submit to CRAN will have to be named However, most R packages on Github are stored under a folder that is the named the same as the package itself. The function Should I delete my current pull request and update the |
I'm guessing that is an issue regardless of whether the subfolder You can simply add any changes to the |
I have updated my With my original structure, you would have to install from Github with: |
@spkaluzny Nice, thanks for doing the research 🎉! That should be more aligned with the way most people download packages from GitHub and the package name is also correct 👍. I'm merging this pull request, we can iterate on the data loading process later on if need be. Thanks for your implementation and documentation! |
A complete package for getting the data from the website and loading the data into an R session. See my note under the Package Design issue.