Closed pegeler closed 6 years ago
Merging #5 into master will decrease coverage by
2.91%
. The diff coverage is4%
.
@@ Coverage Diff @@
## master #5 +/- ##
==========================================
- Coverage 83.33% 80.41% -2.92%
==========================================
Files 9 10 +1
Lines 276 286 +10
==========================================
Hits 230 230
- Misses 46 56 +10
Impacted Files | Coverage Δ | |
---|---|---|
R/api-key.R | 0% <0%> (ø) |
:arrow_up: |
R/nomisr-package.R | 0% <0%> (ø) |
|
R/data_download.R | 91% <50%> (ø) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 079783b...a133309. Read the comment docs.
Hi Evan,
I love that you continue with development even after approval! This is great!
This PR suggests a slightly different way to handle the API keys than what you had originally developed. In this PR, I am making it so that the API key gets brought in as a session option (
nomisr.API.key
) at package load time. Thennomisr_api_key()
can be used to update, either by checking the environment again or prompting for user input (works both with interactive and batch modes).Although I am familiar with package authors manipulating options at package load time (using
.onLoad
) to be used later on, I am not as familiar with the handling of API keys specifically. However, this matches the flavor of how I've seen similar things handled---and I have gone this route for other packages I have created for personal use. I guess this method feels a little cleaner to me, but feel free to take it or leave it.This concept can be fleshed out a little more in future releases, too. For example,
nomisr_api_key()
Also, I added myself as a contributer and reviewer to the DESCRIPTION file. Hope that's not too presumptuous 😉