-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Make shift+enter work in DS and non-DS scenarios #9445
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9445 +/- ##
==========================================
- Coverage 60.93% 60.92% -0.02%
==========================================
Files 529 529
Lines 28580 28576 -4
Branches 4339 4339
==========================================
- Hits 17415 17409 -6
- Misses 10214 10216 +2
Partials 951 951
Continue to review full report at Codecov.
|
@@ -30,9 +34,10 @@ export class CodeExecutionHelper implements ICodeExecutionHelper { | |||
// On windows cr is not handled well by python when passing in/out via stdin/stdout. | |||
// So just remove cr from the input. | |||
code = code.replace(new RegExp('\\r', 'g'), ''); | |||
const interpreter = await this.interpreterService.getActiveInterpreter(resource); | |||
const processService = await this.processServiceFactory.create(resource); |
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.
Why are you using this.interpreterService.getActiveInterpreter(resource);
instead of reverting to the previous code that used an IConfigurationService
instance to get the python path (61a6282#diff-50459a25c7de099fb1ddc1602ce70271)?
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.
getActiveIntepreter should be more up to date than the configuration service AFAIK. @DonJayamanne what do you think?
I messed up, I clicked "request changes" when I only wanted to leave comments 🙈
Kudos, SonarCloud Quality Gate passed!
|
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.
* Conda run doesn't work for normalizing lines or starting a repl * Add news entries * Fix the rest of the linter errors * Fix unit tests * Fix helper tests
Fixes #9437, #9439
Root cause was the normalizeForInterpreter cannot be run (or does it need to be run) with conda run. This wasn't working because of the need to escape \n on the conda run command line.
Additionally, when starting a REPL, conda run cannot be used as it sits there waiting for python to come back. Instead, just plain python must be called.
Testing for this has to be done manually at this point as we don't have a Conda environment on our test machines. See #9443 for supporting that.