-
Notifications
You must be signed in to change notification settings - Fork 38
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
#18: Shelve/Unshelve #96
Conversation
@hdpoliveira What do you think? |
src/commands.ts
Outdated
@command('hg.shelve', { repository: true }) | ||
public async shelve(repository: Repository) { | ||
var options: ShelveOptions = {} | ||
var shelveName = await interaction.inputShelveName(); |
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.
In mercurial the user can specify a name as well as a description for the shelve. Maybe after getting the name we ask for an optional description (this may be done in a different PR)
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.
[sigh] So, the shelve extension hasn't been touched in 7 years and isn't well-suited to tooling. Unlike other hg commands, there's no way to get parsable output from hg shelve --list
. The format is <shelve name><some number of spaces>(<human readable timestamp)<some number of spaces> changes to: <commit message of the change the shelve is based off>\n<description>
. So, no machine-readable timestamp, no commit hash, no consistent field separator. Any parser or regex would be confused by parentheses or linebreaks in the shelve name or description. It turns out TortoiseHG gets around this by bypassing the shelve extension completely and storing patches in its own .hg subfolder! This PR uses an undocumented -q
flag (😬) to just get the list of shelve names. It could be thrown off by linebreaks in the shelve name, but that's a pathological case.
One option is to have the extension parse shelve metadata directly. The file format is straightforward, but obviously the format could be changed from underneath us. If it ever happens, it'll be in a separate PR.
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.
-q is not undocumented, it appears when you write "hg shelve --help --verbose" 😉
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 looks great! The refactoring for the error prompt open log seems really nice, and the functionality too!
We are getting to the point we need to automate testing, formatting and linting, so we reduce the chances of introducing bugs.
I have just two minor change requests and some comments
src/commands.ts
Outdated
@@ -872,6 +872,28 @@ export class CommandCenter { | |||
} | |||
} | |||
|
|||
@command('hg.shelve', { repository: true }) | |||
public async shelve(repository: Repository) { | |||
var options: ShelveOptions = {} |
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.
Use const when possible
} | ||
|
||
async unshelve(options: UnshelveOptions) { | ||
return await this.run(Operation.Shelve, () => this.repository.unshelve(options)); |
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'm not very familiar with the Operation thing, but don't we need one for Unshelve?
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 don't really understand it either. The Git extension uses the same Operation for stash and unstash, so I'm doing the same.
src/interaction.ts
Outdated
@@ -191,14 +191,31 @@ export namespace interaction { | |||
export function warnNoRollback(this: void) { | |||
return window.showWarningMessage(localize('no rollback', "Nothing to rollback to.")); | |||
} | |||
|
|||
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 looks like it doesn't belong.
We should automate code formatting to avoid these in the future.
I created #95 last night to track progress on automated testing. I actually tried getting it working before working on this PR so that I could test these new commands easily, but couldn't get it working properly. |
This adds the following commands: