Skip to content
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

Support a environment variable, #47

Merged
merged 3 commits into from
Sep 29, 2019

Conversation

HughParsonage
Copy link
Collaborator

@HughParsonage HughParsonage commented Sep 26, 2019

This changes the default value of path throughout to consistently support an environment variable R_READABS_PATH.

The advantages of this method are:

  1. Compliance with WRE. (I suspect by using data/ABS as the default, you're technically noncompliant e.g. in the vignette, so it would be good to fix this before CRAN notices).
  2. Allows multiple sessions/projects on the same computer (even across reboots) to use the same path, rather than each using data/ABS by default or requiring each instance of read_abs to specify the same path (which reduces portability).
  3. As you note in the vignette, you have to specify path when selecting subtables. Referring to an environment variable, or encouraging its use, might make that step redundant. There are other enhancements I had in mind if you think this is a positive addition, including some related to add ability to check if local file exists & is up to date #46 .

Note that this is a backwards incompatible change if a user had not specified path in read_abs but did in read_abs_local. (However, if they specified neither, or both, their code should still work.)

I thought about a package option but this can't be easily set between sessions so would not be as useful.

@codecov
Copy link

codecov bot commented Sep 26, 2019

Codecov Report

Merging #47 into master will decrease coverage by 2.3%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #47      +/-   ##
=======================================
- Coverage    92.3%   90%   -2.31%     
=======================================
  Files          14    15       +1     
  Lines         351   360       +9     
=======================================
  Hits          324   324              
- Misses         27    36       +9
Impacted Files Coverage Δ
R/read_abs_local.R 81.25% <ø> (ø) ⬆️
R/read_abs.R 88.13% <ø> (ø) ⬆️
R/read_cpi.R 100% <ø> (ø) ⬆️
R/download_abs.R 100% <ø> (ø) ⬆️
R/extract_abs_sheets.R 93.75% <ø> (ø) ⬆️
R/zzz.R 0% <0%> (ø)

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 aa2d202...083f79b. Read the comment docs.

@MattCowgill
Copy link
Owner

Thanks @HughParsonage, I'll review this as soon as I can (Monday at latest)

@MattCowgill
Copy link
Owner

This is great, thanks @HughParsonage. What are the other things you had in mind?

@MattCowgill MattCowgill merged commit da2bf6e into MattCowgill:master Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants