-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Disable pylint print statement flag + fix venv detection #954
Conversation
const output = await this.processService.exec(pythonPath, ['-c', 'import sys;print(hasattr(sys, "real_prefix"))']); | ||
// hasattr(sys, 'real_prefix') works for virtualenv while | ||
// '(hasattr(sys, 'base_prefix') and sys.base_prefix != sys.prefix))' works for venv | ||
const code = 'import sys\nif hasattr(sys, "real_prefix"):\n print("virtualenv")\nif hasattr(sys, "base_prefix") and sys.base_prefix != sys.prefix:\n print("venv")'; |
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.
Should that be an elif
for the second clause?
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.
Prob doesn't matter, first won't be executed if the second is true I assume.
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.
Better safe than sorry, so I just made the change directly in the PR. 😄
// hasattr(sys, 'real_prefix') works for virtualenv while | ||
// '(hasattr(sys, 'base_prefix') and sys.base_prefix != sys.prefix))' works for venv | ||
const code = 'import sys\nif hasattr(sys, "real_prefix"):\n print("virtualenv")\nif hasattr(sys, "base_prefix") and sys.base_prefix != sys.prefix:\n print("venv")'; | ||
const output = await this.processService.exec(pythonPath, ['-c', code]); |
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.
Time to change this to iif
Codecov Report
@@ Coverage Diff @@
## master #954 +/- ##
==========================================
- Coverage 64.01% 64.01% -0.01%
==========================================
Files 260 260
Lines 12028 12027 -1
Branches 2131 2130 -1
==========================================
- Hits 7700 7699 -1
Misses 4319 4319
Partials 9 9
Continue to review full report at Codecov.
|
Due to a bug in Pylint where specifying `E` as enabled checks flips on `--py3k`, we need to explicitly list all `E` checkers to explicitly avoid (at least) print-statement which throws false-positives for folks not porting to Python 3. Closes microsoft#722 by partially reverting microsoft#954
Fixes #722
Properly detects venv
Added tests