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.

Sign up for GitHub

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

Jump to bottom

Avoid clobbering quick-query file when re-opened #747

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Avoid clobbering quick-query file when re-opened #747

merged 1 commit into from
Feb 22, 2021

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Feb 8, 2021 •
edited
Loading

Only recreate the qlpack.yml file.

Also, add an integration test for quick-query creation.

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.

adityasharad
adityasharad reviewed Feb 8, 2021
View reviewed changes
extensions/ql-vscode/src/vscode-tests/cli-integration/queries.test.ts Outdated
@@ -21,7 +22,7 @@ import { QueryResultType } from '../../pure/messages';
* Integration tests for queries
*/
describe('Queries', function() {
this.timeout(20000);
this.timeout(20000000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove that.

extensions/ql-vscode/src/quick-query.ts Outdated
@@ -109,24 +106,22 @@ export async function displayQuickQuery(
libraryPathDependencies: [qlpack]
};

// Only recreate the query file if it doesn't already exist.
// Always recreate the qlpack file because the file will change when database languages change
Copy link
Contributor

Choose a reason for hiding this comment

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

If the database language changes, then the existing query file may not be useful (especially if it contains import <language>). Can we detect whether the language has changed, and replace both query and qlpack files only in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to avoid the complexity, but maybe because of the import statement, we do want to overwrite the old file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Simpler: always overwrite both query and qlpack files whenever you switch database. No need to check the language.

This is suboptimal if you're writing a quick query for the same language and want to run it on several different DBs. But it's an improvement on what we currently have. If the user needs the old query they can still get it from query history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The complexity is more that the user may change databases and keep the quick-query file open. When that happens, the contents will be wrong. The quick-query contents are only recreated when the command is called.

I guess this incorrect behaviour is currently in the extension. In order to fix this, I need to add a listener for database changes. We already have these listeners, so it's not too bad.

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 that's ok? If they have the query open we ignore it. If they ask for a fresh Quick Query we give them a fresh one if it's a new DB, or the existing one if the DB hasn't changed. Seems like a strict win without having to add more listeners, though if you think that's worthwhile I don't mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's getting messy. I'll change things so that the quick query is only updated when the database changes (if necessary).

extensions/ql-vscode/src/quick-query.ts Show resolved Hide resolved
@nickrolfe
Copy link
Contributor

I downloaded the .vsix and had a quick play, and it's definitely a nice QoL improvement for me. Thanks!

@aeisenberg
Copy link
Contributor Author

Discovered a bug in quick-queries (exists in current implementation as well). I'll fix this bug in this PR.

The bug is that sometimes even after you close the quick query window, the query text document stays in memory. Our check for whether or not to recreate the quick query fails (ie- the extension thinks the query window has remained open, even though it is closed, so the quick-queries are not recreated).

I'll make the fix in this PR, but in a different commit.

@aeisenberg aeisenberg force-pushed the aeisenber/quick-query branch 5 times, most recently from bab4d9e to c5ec85c Compare February 11, 2021 21:10
@aeisenberg aeisenberg force-pushed the aeisenber/quick-query branch 3 times, most recently from c5bac56 to 301f388 Compare February 22, 2021 16:52
@aeisenberg
Copy link
Contributor Author

Hmmm...I need to figure out this flaky test failure. It is passing locally.

@aeisenberg
Copy link
Contributor Author

OK...this is the same problem that we originally had in the telemetry tests. Should be fixed by adding a short wait after updating the settings. In the future, I should refactor all wait() functions into a single, shared instance. That's for later, though.

adityasharad
adityasharad approved these changes Feb 22, 2021
View reviewed changes
extensions/ql-vscode/src/quick-query.ts Outdated
}
}

async function checkShouldRewrite(qlPackFile: string, newDepedency: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
async function checkShouldRewrite(qlPackFile: string, newDepedency: string) {
async function checkShouldRewrite(qlPackFile: string, newDependency: string) {

extensions/ql-vscode/src/quick-query.ts Outdated Show resolved Hide resolved
extensions/ql-vscode/src/quick-query.ts
}
const qlPackContents: any = yaml.safeLoad(await fs.readFile(qlPackFile, 'utf8'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth having a simple type here instead of any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it adds much value here. If we are going to start using qlpack files elsewhere in the code, then yes.

@aeisenberg
Avoid clobbering quick-query file when re-opened
07a5925 
Only recreate the qlpack.yml file.

Also, add an integration test for quick-query creation.
@aeisenberg aeisenberg enabled auto-merge (rebase) February 22, 2021 19:59
@aeisenberg aeisenberg merged commit 8396655 into github:main Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adityasharad adityasharad adityasharad approved these changes

@nickrolfe nickrolfe Awaiting requested review from nickrolfe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aeisenberg @nickrolfe @adityasharad