Implement "open query directory" for variant analysis history items#1622
Implement "open query directory" for variant analysis history items#1622shati-patel merged 1 commit intomainfrom
Conversation
| } else if (finalSingleItem.t === 'remote') { | ||
| externalFilePath = path.join(this.queryStorageDir, finalSingleItem.queryId, 'timestamp'); | ||
| } else if (finalSingleItem.t === 'variant-analysis') { | ||
| externalFilePath = path.join(this.variantAnalysisManager.storagePath, finalSingleItem.variantAnalysis.id.toString(), 'timestamp'); |
There was a problem hiding this comment.
We don't yet create a timestamp file for variant analysis items, so we always fall back to this block:
if (!(await fs.pathExists(externalFilePath))) {
// timestamp file is missing (manually deleted?) try selecting the parent folder.
// It's less nice, but at least it will work.
externalFilePath = path.dirname(externalFilePath);This works fine, but should we be creating a timestamp file? (see https://github.com/github/vscode-codeql/blob/main/extensions/ql-vscode/src/helpers.ts#L560-L564)
There was a problem hiding this comment.
I don't have full context but seems like we probably should create a timestamp file for new variant analyses. Seems we do this in the remote queries case by calling await this.prepareStorageDirectory(queryId); just before we trigger the monitoring command.
But as you explain, this is semi-independent of this PR because there's a fallback that covers us. So I'd suggest we add thetimestamp file but it doesn't necessarily have to be in this PR or block this PR.
| } else if (queryHistoryItem.t === 'remote') { | ||
| return path.join(this.queryStorageDir, queryHistoryItem.queryId); | ||
| } else if (queryHistoryItem.t === 'variant-analysis') { | ||
| return path.join(this.variantAnalysisManager.storagePath, queryHistoryItem.variantAnalysis.id.toString()); |
There was a problem hiding this comment.
Can we make use of something like VariantAnalysisResultsManager.getStorageDirectory. Would need to make that method public, and expose the results manager through the variant analysis manager. Would appreciate thoughts on what the architecture of this should look like.
There was a problem hiding this comment.
It feels like the variant analysis manager should know and be able to tell others about the storage location for a variant analysis. So I'd have:
variantAnalysisManager.getVariantAnalysisStorageLocation(variantAnalysisId: number): string {}
and then call that from query history.
There was a problem hiding this comment.
It does seem that this, along with the timestamp is something that should be done in a separate predecessor PR/issue.
There was a problem hiding this comment.
Fine by me to do it in a separate PR, either before or after this PR. 👍
There was a problem hiding this comment.
Good point - I'm going to convert this PR to a draft for now and get the foundational stuff done in a separate PR first!
There was a problem hiding this comment.
@shati-patel ok, I'll not do it here then: #1621
|
Prep work in #1631! 🧹 |
shati-patel
force-pushed
the
shati-patel/open-query-dir
branch
from
9a4ac13 to
dab67f1
Compare
|
Thank you for the patience 😅 Rebased this on top of the changes from #1631 and tested that the query history command works. This should be actually ready for review now! |
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Yay! Thank you for making the other changes
4 participants
Adds the ability to open the query directory for a variant analysis history item. The "Open Query Directory" command in the query history context menu should now work for all kinds of history items 🎉
❓ Note: I've left a question inline about whether we need a
timestampfile.Checklist
N/A - internal only 🐘
ready-for-doc-reviewlabel there.