-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Backup system #581
Backup system #581
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Great initiative. :)
Should we add a config setting to disable backups in case people want to do that? Just an idea. Apart from that, I have very minor remarks, which are not blockers.
const backup = this.backups.find((backup) => backup.id === id); | ||
|
||
if (!backup) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a log message here?
src/extension.ts
Outdated
if (process.env.NODE_ENV === "test") { | ||
ctx.subscriptions.push( | ||
vscode.commands.registerCommand( | ||
"snippet.test_moveElement", | ||
async (sourceId, targetId) => | ||
await snippetsStorage.moveElement(sourceId, targetId) | ||
) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you planning to keep this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so the tests for the backup system check that when we rename
, save
, delete
or move
snippets.
rename
, save
and delete
are exposed as commands, so we can call them in tests (we don't directly instantiate different classes such as BackupManager
and don't call their API in tests, but instead interact with the extension via commands, like the real user will do).
But the move
is executed only via drag-and-drop (in the SnippetsTreeProvider
class, the handleDrop
method calls storage.moveElement
). So, we need to call storage.moveElement
in tests but it is not exposed outside, it can only be called via handleDrop
, and I haven't found how to call it in tests.
So I decided to expose storage.moveElement
as a command (but only in test environment: process.env.NODE_ENV === "test"
). This looks like a hack but I haven't found a better solution
src/snippetsStorage.ts
Outdated
this.onSnippetSave?.(element); | ||
} | ||
|
||
getSnippet(id: string): string { | ||
return this.getElement(id).data.content?.toString() || ""; | ||
} | ||
|
||
async save(): Promise<void> { | ||
async save(operation?: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if operation
could be an enum. This way, we can the operations get type checked and spelling mistakes get prevented.
Looks good. Amazing work! |
@mre, the PUBLISHER_TOKEN has expired, could you please, when you have time, generate a new token and replace the existing one with new? (https://aka.ms/vscodepat) |
Oh, okay. |
Ah, that's for the Open VSX registry. Also, the GitHub releases and the package.json are out of sync right now. Perhaps we should try to unpublish the VSX 1.1.6 release and change the |
Currently users can backup snippets either by using the built-in VSCode Sync backups (but only if VSCode Sync is enabled) or by creating a backup copy of the
state.vscdb
file (I described both of these cases in the README).However, I thought that it would be useful to have a simple, built-in backup system.
It saves snippets locally on the user's computer and creates up to 10 backups on
save
,delete
,move
orrename