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

Add query history sorting for remote queries #1235

Merged
merged 3 commits into from
Mar 29, 2022
Merged

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Mar 25, 2022

Also, fix two smaller issues:

  • Ensure the Open Query Directory command opens inside the specified
    directory.
  • Ensure label changes are saved across restarts.

Fixes #1204

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [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.

Also, fix two smaller issues:

- Ensure the `Open Query Directory` command opens inside the specified
  directory.
- Ensure label changes are saved across restarts.
@aeisenberg aeisenberg requested a review from a team as a code owner March 25, 2022 21:26
Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me! Just a few minor comments 🎉

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

- Fix a bug where the AST viewer was not synchronizing its selected node when the editor selection changes. [#1230](https://github.com/github/vscode-codeql/pull/1230)
- Open the directory in the finder/explorer (instead of just highlighting it) when running the command "Open query directory" command from the query history view. [#1235](https://github.com/github/vscode-codeql/pull/1235)
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo ✍🏽

Suggested change
- Open the directory in the finder/explorer (instead of just highlighting it) when running the command "Open query directory" command from the query history view. [#1235](https://github.com/github/vscode-codeql/pull/1235)
- Open the directory in the finder/explorer (instead of just highlighting it) when running the "Open query directory" command from the query history view. [#1235](https://github.com/github/vscode-codeql/pull/1235)

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

- Fix a bug where the AST viewer was not synchronizing its selected node when the editor selection changes. [#1230](https://github.com/github/vscode-codeql/pull/1230)
- Open the directory in the finder/explorer (instead of just highlighting it) when running the command "Open query directory" command from the query history view. [#1235](https://github.com/github/vscode-codeql/pull/1235)
- Ensure query label changes are persisted across restarts. [#1235](https://github.com/github/vscode-codeql/pull/1235)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can clarify what these labels are 💡 Something like this?

Suggested change
- Ensure query label changes are persisted across restarts. [#1235](https://github.com/github/vscode-codeql/pull/1235)
- Ensure query label changes in the query history view are persisted across restarts. [#1235](https://github.com/github/vscode-codeql/pull/1235)

}
} else if (finalSingleItem.t === 'remote') {
p = path.join(this.queryStorageDir, finalSingleItem.queryId);
externalFilePath = path.join(this.queryStorageDir, finalSingleItem.queryId, 'timestamp');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of the timestamp file! ⏲️

}

if (p) {
if (externalFilePath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably a fringe case, but is it worth also throwing a Failed to open error when this path doesn't exist (e.g. if a user has deleted the timestamp for some reason)?
At the moment it fails silently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm...unfortunately, the revealFileInOS command does not throw an error if the file doesn't exist. I guess I can add a fs.pathExists check before trying to open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Also, fix some changelog notes.
@aeisenberg
Copy link
Contributor Author

@shati-patel can you take another look and see if your comment is addressed?

Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! LGTM 🙌🏽

@aeisenberg aeisenberg merged commit 49cceff into main Mar 29, 2022
@aeisenberg aeisenberg deleted the aeisenberg/history-sort branch March 29, 2022 18:13
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.

Show Query Directory opens parent folder
2 participants