Skip to content

Expand description for codeQL.cli.executablePath#2441

Merged
aeisenberg merged 1 commit intomainfrom
RasmusWL/codeql-executable-wording
Jun 9, 2023
Merged

Expand description for codeQL.cli.executablePath#2441
aeisenberg merged 1 commit intomainfrom
RasmusWL/codeql-executable-wording

Conversation

@RasmusWL
Copy link
Copy Markdown
Member

@RasmusWL RasmusWL commented May 23, 2023

To match what the code actually does.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request. -- no user visible changes, so has not been updated
  • Issues have been created for any UI or other user-facing changes made by this pull request. -- no user visible changes, so has no issue has been created
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

To match what the code actually does.
@RasmusWL RasmusWL requested a review from a team as a code owner May 23, 2023 10:05
"type": "string",
"default": "",
"markdownDescription": "Path to the CodeQL executable that should be used by the CodeQL extension. The executable is named `codeql` on Linux/Mac and `codeql.exe` on Windows. If empty, the extension will look for a CodeQL executable on your shell PATH, or if CodeQL is not on your PATH, download and manage its own CodeQL executable."
"markdownDescription": "Path to the CodeQL executable that should be used by the CodeQL extension. The executable is named `codeql` on Linux/Mac and `codeql.exe` on Windows. If empty, the extension will look for a CodeQL executable on your shell PATH, or if CodeQL is not on your PATH, download and manage its own CodeQL executable (note: if you later introduce CodeQL on your PATH, the extension will prefer a CodeQL executable it has downloaded itself)."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this only apply if the setting is empty, or also if you set this to an explicit path? I assume the first option otherwise there's no way to stop using the managed executable.

Is there any way to get the extension to stop using the managed executable and switch to the one on the path?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if the VS Code setting is empty.

The logic is here

It goes in priority, and picks the first option that is set:

  1. customCodeQlPath set in VS Code settings
  2. extensionSpecificCodeQlPath (installation managed by extension)
  3. Look through PATH

Is there any way to get the extension to stop using the managed executable and switch to the one on the path?

I guess you can just delete the folders containing the CodeQL CLI downloaded by the extension, but otherwise no.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining. This makes me wonder why we use this priority because at first glance it doesn't seem to most friendly, and is hard to override at a later date. Not that I'm blaming you, or expect you to change anything. But it's interesting to highlight what the current behaviour actually is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit wonky. I remember at the time of developing the extension we wanted to support these three paths, but I'm not convinced the current implementation was exactly the intent 😆 All the same, documenting now sounds great, and we can figure out how to improve it later in a way that minimises breakage.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It goes in priority, and picks the first option that is set:
1. customCodeQlPath set in VS Code settings
2. extensionSpecificCodeQlPath (installation managed by extension)
3. Look through PATH

It looks to me that steps 2 and 3 should be reversed.

@RasmusWL RasmusWL requested a review from robertbrignull June 8, 2023 09:56
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any complaints about this PR. Documenting the current behaviour a bit more accurately is definitely good, so I think this PR is for sure beneficial.

It has highlighted that the current behaviour is a bit odd and arguably not very good in certain cases. But that doesn't mean we shouldn't document it.

@aeisenberg aeisenberg closed this Jun 8, 2023
@aeisenberg aeisenberg reopened this Jun 8, 2023
@aeisenberg aeisenberg enabled auto-merge June 8, 2023 19:50
@aeisenberg aeisenberg merged commit afb490b into main Jun 9, 2023
@aeisenberg aeisenberg deleted the RasmusWL/codeql-executable-wording branch June 9, 2023 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants