Avoid clobbering quick-query file when re-opened#747
Conversation
bb270a0 to
c3bc218
Compare
| */ | ||
| describe('Queries', function() { | ||
| this.timeout(20000); | ||
| this.timeout(20000000); |
There was a problem hiding this comment.
Forgot to remove that.
| }; | ||
|
|
||
| // 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I was hoping to avoid the complexity, but maybe because of the import statement, we do want to overwrite the old file.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No, that's getting messy. I'll change things so that the quick query is only updated when the database changes (if necessary).
|
I downloaded the |
|
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. |
bab4d9e to
c5ec85c
Compare
c5bac56 to
301f388
Compare
|
Hmmm...I need to figure out this flaky test failure. It is passing locally. |
301f388 to
67c55a7
Compare
|
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 |
| } | ||
| } | ||
|
|
||
| async function checkShouldRewrite(qlPackFile: string, newDepedency: string) { |
There was a problem hiding this comment.
| async function checkShouldRewrite(qlPackFile: string, newDepedency: string) { | |
| async function checkShouldRewrite(qlPackFile: string, newDependency: string) { |
| if (!(await fs.pathExists(qlPackFile))) { | ||
| return true; | ||
| } | ||
| const qlPackContents: any = yaml.safeLoad(await fs.readFile(qlPackFile, 'utf8')); |
There was a problem hiding this comment.
Worth having a simple type here instead of any?
There was a problem hiding this comment.
I don't think it adds much value here. If we are going to start using qlpack files elsewhere in the code, then yes.
Only recreate the qlpack.yml file. Also, add an integration test for quick-query creation.
67c55a7 to
07a5925
Compare
Only recreate the qlpack.yml file.
Also, add an integration test for quick-query creation.
Checklist
@github/docs-content-dsphas been cc'd in all issues for UI or other user-facing changes made by this pull request.