-
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
Add conda run without output #17982
Add conda run without output #17982
Conversation
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.
Note that linter output changes with quotes, so we may have to update the linter code to parse this correctly:
(.venv) C:\GIT\repro\scratch>flake8 --format=%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s something2.py
1,1,F,F401:'json' imported but unused
2,1,F,F401:'os' imported but unused
4,1,F,F821:undefined name 'x'
(.venv) C:\GIT\repro\scratch>flake8 --format='%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s' something2.py
'1,1,F,F401:'json' imported but unused'
'2,1,F,F401:'os' imported but unused'
'4,1,F,F821:undefined name 'x''
19acbe2
to
df92097
Compare
a5b6425
to
bc2713c
Compare
@@ -14,7 +14,7 @@ export class Flake8 extends BaseLinter { | |||
|
|||
protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> { | |||
const messages = await this.run( | |||
['--format=%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', document.uri.fsPath], | |||
['--format= %(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', document.uri.fsPath], |
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.
What does adding a space here (and in src/client/linters/pycodestyle.ts) do?
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.
it makes all the value inside "", "--format=%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s"
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.
It turns the single quote into double quotes? Could you explain how adding a space in the argument does that? I don't understand 😅
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.
I'm not very sure, but internally when you pass a parameter by array to the run function, if it has any special character or spaces it renders it with "". The same thing happens with path directory, when the name has a space, it appears in the console with "".
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.
that happens because we quote arguments if there is a space in them. We have code that adds quote if the path or arguments have some characters.
2fdcb6a
to
90a8349
Compare
Co-authored-by: Kartik Raj <[email protected]>
…o25/vscode-python into Add-Conda-run-without-output
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.
We should avoid introducing the option bypassCondaExecution
if possible. Also, can you test linting, installing etc. to make sure it works with a conda environment?
@@ -117,11 +126,17 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { | |||
processService.on('exec', this.logger.logProcess.bind(this.logger)); | |||
this.disposables.push(processService); | |||
|
|||
// Allow parts of the code to ignore conda run. |
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 PR it seems it's only used for tests, can you elaborate where this is needed?
@@ -296,7 +296,7 @@ suite('Installer', () => { | |||
} | |||
callback({ stdout: '' }); | |||
}); | |||
await installer.isInstalled(product, 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.
What happens if you do not bypass conda execution here, ideally I would expect the tests to pass even in that case. Can you attempt to fix the test instead of using this option?
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.
The tests were throwing a timeout error, I tried to fix it but it wasn't consistent and it was throwing another error, that's why I put the timeout.
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.
The tests were throwing a timeout error
I assume this is on CI as tests seem to be passing locally for me. It's fine to put the timeout or even increase timeout, but I think we shouldn't need to bypass conda execution. It's weird as conda execution should not even be used if we're not using a conda interpreter.
Implemented here #18319 |
Add conda run support
Closed #17973