-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat(plugins): implement new-remove command #247
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
Conversation
|
658efc7 to
bbe489d
Compare
|
@copilot please fix the linting issues now when I've rebased this PR (the "no-await-in-loop" rule can be disabled for the whole file, like it's done for the "new-up.ts" file). Also change the logic of the "new-remove" command so that migrations with the wrong name are not skipped but instead just ignored (just do |
Fixed in commit a2d24ac:
|
joakimbeng
left a comment
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.
@copilot only one little thing
|
@copilot Fix the lint issue |
| // The lock call is required by the context's state machine to enable execution | ||
| await context.lock(toRemove); |
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.
This is not correct and should be removed. The remove method does not require locking the migration before removing it
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.
Fixed in commit 1911183. Removed the unnecessary lock call and its comment.
| for (const migration of toRemove) { | ||
| await context.remove(migration); | ||
| } |
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.
As there will only be one migration to remove, the await context.remove(migration) call can be moved inside the for loop above, i.e. by replacing the toRemove.push(migration) line.
And then it can be followed by a break statement for a fast finish of the loop
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.
Fixed in commit 1911183. Moved await context.remove(migration) inside the for loop and added a break statement for fast finish.
|
@copilot address my latest comments |
1911183 to
8bc33de
Compare
Co-authored-by: joakimbeng <1427383+joakimbeng@users.noreply.github.com>
…tering logic Co-authored-by: joakimbeng <1427383+joakimbeng@users.noreply.github.com>
Co-authored-by: joakimbeng <1427383+joakimbeng@users.noreply.github.com>
Co-authored-by: joakimbeng <1427383+joakimbeng@users.noreply.github.com>
Co-authored-by: joakimbeng <1427383+joakimbeng@users.noreply.github.com>
8bc33de to
d812cae
Compare
new-remove.ts- the remove command implementationnew-remove.test.ts- comprehensive tests for the remove command (24 tests)assertStorageRemovedhelper intests/storage.tsMockedFinishedtype intests/plugin.tsto include 'removed' status/* eslint-disable no-await-in-loop */at top of file like new-up.tsfor...ofinstead of.forEach()inassertStorageRemoved(unicorn/no-array-for-each)continue) instead of skippedlockcall (remove doesn't require locking)context.remove()call inside the for loop withbreakfor fast finish💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.