-
Notifications
You must be signed in to change notification settings - Fork 884
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
Ensure config dir is writable #135
Conversation
… permissions error
Codecov Report
@@ Coverage Diff @@
## master #135 +/- ##
==========================================
- Coverage 88.76% 88.68% -0.09%
==========================================
Files 75 76 +1
Lines 7803 7845 +42
==========================================
+ Hits 6926 6957 +31
- Misses 877 888 +11
Continue to review full report at Codecov.
|
@@ -8,4 +8,4 @@ jobs: | |||
- checkout | |||
- run: pyenv local 2.7.13 3.5.2 3.6.0 | |||
- run: make installdeps | |||
- run: make lint && tox && codecov | |||
- run: tox && make lint && codecov |
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.
any reason for shifting order? I'd think we want things to fail faster
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.
My thought was that you'd want to know if your code passed the tests regardless of whether it passed linting. That is a more important indicator of how your branch is doing.
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.
In this case in particular, the lint error was masking the actual testing error that I cared about, and was not able to reproduce locally
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.
gotcha. this change makes sense to me
Looks good. Merging |
Issue: on some environments (namely, some automatic CI environments) the home directory is not writable. Featuretools thus fails to install because we attempt to create a ~/.featuretools folder.
This PR resolves this problem by checking if the config folder is writable, and if not, it creates a temporary folder instead. It also allows a user to specify which folder to use for config via an environment variable ($FEATURETOOLS_DIR).
Much of this code was modified from the IPython source:
https://github.com/ipython/ipython/blob/master/IPython/utils/path.py (get_ipython_dir())
and
https://github.com/ipython/ipython/blob/master/IPython/paths.py (get_home_dir())
Lastly, it changes config_yaml.txt to a proper yaml file: config.yaml