Skip to content
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

Pass computeDefaultStrings to query server when compiling queries #694

Merged
merged 3 commits into from
Dec 1, 2020

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Nov 30, 2020

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • [n/a] Issues have been created for any UI or other user-facing changes made by this pull request.
  • [n/a] @github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Nice. I suggest adding a query integration test.

* Whether to ensure that elements that do not have a displayString
* get reported anyway. Useful for universal compilation options.
*/
computeDefaultStrings: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Future work: This makes me think we should have an internal integration test, which checks the external and internal copies of this protocol are in sync.

@adityasharad
Copy link
Contributor

Worth adding a short changelog entry too, since it's actually user-visible. Something like "Query results without a string representation were previously not computed. They will now be computed and displayed with a default placeholder string."

@aeisenberg
Copy link
Contributor Author

Test is added and and query tests are now running in CI.

@@ -3,6 +3,7 @@
## [UNRELEASED]

- Fix bug when removing databases where sometimes the source folder would not be removed from the workspace or the database files would not be removed from the workspace storage location. [#692](https://github.com/github/vscode-codeql/pull/692)
- Always pass the `computeDefaultStrings` flag to query compilation commands. This will change the output of some queries. [#694](https://github.com/github/vscode-codeql/pull/694)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I put this into an earlier comment I can't find - I'd prefer to mention what the user-facing change here is, i.e. query results with no string representation were previously omitted, and will now be displayed with placeholder text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I didn't quite know how to say it. This sounds good.

@aeisenberg aeisenberg force-pushed the aeisenberg/computeDefaultStrings branch from 0f7074b to 4b4a12b Compare December 1, 2020 22:20
@aeisenberg aeisenberg force-pushed the aeisenberg/computeDefaultStrings branch from 4b4a12b to ed5dc69 Compare December 1, 2020 22:23
@aeisenberg aeisenberg merged commit 3c08baf into github:main Dec 1, 2020
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.

2 participants