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

Change "Get Online Help" menu item label: remove Online. #1516

Merged
merged 4 commits into from
Sep 21, 2018
Merged

Change "Get Online Help" menu item label: remove Online. #1516

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9211bc2
Change Menu label: remove Online.
corbob Sep 3, 2018
e0352db
Rename the file to ShowHelp. Continue removing word Online
corbob Sep 16, 2018
8593ebf
Remove Dummy command. Cleanup structure.
corbob Sep 19, 2018
9b3ece5
Remove showOnlineHelp requestType as it's not used.
corbob Sep 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
},
{
"command": "PowerShell.OnlineHelp",
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume changing the title would be a breaking or otherwise undesirable change?

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 wasn't wanting to change the internal messages in case I missed one. Unless someone is using the command palette to run PowerShell.OnlineHelp directly, then this shouldn't be a breaking change...

That being said, I used search and replace across the workspace (PSES and vscode-powershell) to find and replace all of the instances that referred to OnlineHelp.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the command should be called PowerShell.ShowHelp, ShowOnlineHelp.ts renamed to ShowHelp.ts, etc. The breaking change I think would come from changing the LSP message that PSES expects. In that case, couldn't we create handle a new message powerShell/showHelp while also handling the original message powerShell/showOnlineHelp?

Copy link
Member

Choose a reason for hiding this comment

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

@rkeithhill I think we were going off of the idea that vscode is likely the only editor that takes advantage of this message at this time so the change was lower risk since we control both.

That said, I'll stand by you if you feel like we should do the is right way and handle the breaking change correctly (by not having a breaking change 😄 )

Copy link
Contributor

Choose a reason for hiding this comment

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

@rkeithhill Yes that seems like the right behaviour. PSES should support a deprecated powerShell/showOnlineHelp message as an alias of powerShell/showHelp I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if I'm reading this correctly, @rkeithhill is suggesting I change all of the things I already changed (and stripped out of commit history because we said not to), and then on the PSES side of things have the same handler accept both powerShell/showOnlineHelp and powerShell/showHelp?

If that's the way we want to go, I'm game for doing it. But this time around I'm going to wait for the thumbs up 😜

Copy link
Contributor

@rkeithhill rkeithhill Sep 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How about we leave the "command" name as-is for 1.x - leave it at PowerShell.OnlineHelp. In theory, it could be used by others via the VSCode API that allows you invoke a command by name. But I think everything else (ShowOnlineHelp.ts filename, etc) should be changed to reflect the new semantics of the command.

Then in v2, let's rename the command to PowerShell.ShowHelp and document it as a breaking change.

then on the PSES side of things have the same handler accept both powerShell/showOnlineHelp and powerShell/showHelp?

Yes. And in v2 we could remove the powerShell/showOnlineHelp message and document the breaking change there.

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've modified both sides to note the deprecation of showOnlineHelp. Currently in vscode-powershell is a command that actually sends this message to PSES so that I could test the warning on PSES's side.

I think with the way the code is now, we should be able to mark it as deprecated, and when ready to remove as breaking change it's just a matter of removing it. From experience: I prefer this type of warning before removal because a lot of people don't read the change notes (unfortunately) and then freak out even if it was noted as a breaking change.

"title": "Get Online Help for Command",
"title": "Get Help for Command",
"category": "PowerShell"
},
{
Expand Down