-
Notifications
You must be signed in to change notification settings - Fork 2
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
Move PTAXSIM DB file to runner temp dir #36
Conversation
@@ -13,7 +13,7 @@ knitr::opts_chunk$set( | |||
|
|||
# Introduction | |||
|
|||
Property tax exemptions are savings that lower a homeowner’s property tax bill. They work by reducing or freezing the taxable value (Equalized Assessed Value, or EAV) of a home. For example, the most common exemption, the Homeowner Exemption, reduces EAV by \$10,000. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff is just extraneous cleanup from my built-in linter/fixers.
```{r, echo=FALSE} | ||
# This is needed to build the vignette using GitHub Actions | ||
ptaxsim_db_conn <- DBI::dbConnect( | ||
RSQLite::SQLite(), | ||
Sys.getenv("PTAXSIM_DB_PATH") | ||
) | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vignettes need to point at the temp directory during build time, but I don't necessarily want to use the env var setup here since it's a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, looks great! I don't totally get the different circumstances under which the database connection is supposed to be created for the vignettes, but I'm fine moving forward if you feel good about this pattern.
# This is needed to build the vignette using GitHub Actions | ||
ptaxsim_db_conn <- DBI::dbConnect( | ||
RSQLite::SQLite(), | ||
Sys.getenv("PTAXSIM_DB_PATH") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question, non-blocking] What's the relationship between this DBI::dbConnect()
call and the one that precedes it on line 50? Is the idea just that this will overwrite the preceding ptaxsim_db_conn
definition on CI, but locally we can skip this call and use the one on line 50 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got it. This call is hidden in the actual output vignette HTML just because I think it's simpler to show a file path instead of an env var setup, but the env var is necessary for it to work on CI.
This PR fixes two bugs related to our PTAXSIM CI integration. Mainly:
NaN
tax rate (divide by 0 error). I added a line to replaceNaN
values with 0 and added a test to catch future issues of the same nature.