-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
chore: starter make file #13970
chore: starter make file #13970
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13970 +/- ##
==========================================
+ Coverage 77.87% 78.65% +0.77%
==========================================
Files 935 935
Lines 47375 47375
Branches 5964 5964
==========================================
+ Hits 36895 37262 +367
+ Misses 10323 9980 -343
+ Partials 157 133 -24
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at 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.
This is awesome!
|
||
venv: | ||
# Create a virtual environment and activate it (recommended) | ||
python3 -m venv venv # setup a python3 virtualenv |
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.
Later we should add a check
target that runs before all other targets, verifying that the user has the proper Python version installed.
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.
Have we considered naming the superset virtual environment something other than venv
(perhaps specific to this application)? This could reduce the chance it will conflict with existing virtual environments on folks' machines.
Cloned a fresh repo and ran
|
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.
Like Rob, I got the same error when running from my machine:
# Create a virtual environment and activate it (recommended)
python3 -m venv venv # setup a python3 virtualenv
source venv/bin/activate
make: source: No such file or directory
make: *** [venv] Error 1
I assume this has to do with my use of the virtualenv
plugin for pyenv
. Link to the GH page. I installed it with brew
.
Perhaps if we detected this condition we could make the requisite virtualenv
using this plugin via pyenv virtualenv-init
. This is not enough of a blocker for me to request changes, but wanted to bring it up for consideration.
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.
If possible, it'd be good to make these commands applicable to a wide range of environments. I wonder if we can solve the issue with sourcing the virtual env?
If there's time, I agree with @betodealmeida that checking for proper pre-requisites and rendering a helpful message would be 👌🏻
|
||
venv: | ||
# Create a virtual environment and activate it (recommended) | ||
python3 -m venv venv # setup a python3 virtualenv |
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.
Have we considered naming the superset virtual environment something other than venv
(perhaps specific to this application)? This could reduce the chance it will conflict with existing virtual environments on folks' machines.
Going to merge for superset install, but going to look into the venv install and checking for prerequisites in the next PR. |
* starter make file * yea * update docs * lit * remove venv from comamnds * venv
SUMMARY
Using this PR to start enhancing our dev env setup! Developers will now be able to run
make install
to install superset and pre-commit envBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION