-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add option to manually override data directory #71
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
- Coverage 92.98% 92.98% -0.01%
==========================================
Files 15 16 +1
Lines 3351 3363 +12
==========================================
+ Hits 3116 3127 +11
- Misses 235 236 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM
end | ||
end | ||
|
||
const DATA_DIR = get_data_dir() |
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.
Are you sure about this? Won’t it get cached the first time and then not re-evaluated?
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.
Yes, it will, and this may necessitate another approach — see the initial PR comment above.
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.
Sorry, didn’t read that all the way through - or your comment above the function. I think the current behavior will be too confusing and that you will have to replace every use of DATA_DIR with a function.
Fixes #54. Here I have opted to check for the environment variable at the point where the existing
const DATA_DIR
is defined. This gets bound up in the precompilation such that if a user wants to redefine the data directory after precompilation, they would have to manually re-compile withBase.compilecache(Base.identify_package("PowerSystemCaseBuilder"))
. If that is acceptable, I'll go ahead and write the documentation for how to use this new feature. If not, we will probably have to replace uses ofDATA_DIR
with calls to a function that checks the environment variable each time; I am not opposed to this but I thought I'd be conservative with my first pass.