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

corbob · 2018-09-03T13:31:30Z

PR Summary

Due to PowerShell/PowerShellEditorServices#721 we are now looking for help locally if it's not available online. This renames the menu option to be "Get Help" instead of "Get Online Help"

Point of discussion: Should Online be removed from the internal messages/function names?

PR Checklist

PR has a meaningful title
Summarized changes
This PR is ready to merge and is not work in progress

We now look for help locally if it's not availble online. Perhaps to do: remove references to Online from internal commands?

rjmholt · 2018-09-06T03:47:49Z

package.json

@@ -88,7 +88,7 @@
      },
      {
        "command": "PowerShell.OnlineHelp",


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

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.

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?

@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 😄 )

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

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 😜

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.

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.

TylerLeonhardt

LGTM 🎉 assuming this comes out before 2.0, this should be apart of 1.9 since it has a breaking change.

rjmholt · 2018-09-06T04:52:11Z

I left a comment in the PSES PR (PowerShell/PowerShellEditorServices#721 (comment)) about the breaking change, but this looks good

rjmholt

LGTM!

TylerLeonhardt

Still looks good 👍

Add a command for PowerShell.ShowHelp and mark OnlineHelp as deprecated. Display warning when OnlineHelp is called and then execute ShowHelp.

TylerLeonhardt

just a little clean up :) but looks good!

TylerLeonhardt · 2018-09-18T02:45:54Z

src/features/ShowHelp.ts

+
+export const ShowOnlineHelpRequestType =
+    new RequestType<string, void, void, void>("powerShell/showOnlineHelp");
+    // I think we can take out the ShowOnlineHelpRequestType... I don't think we actually send it...


re-word this comment to to:

TODO: remove ShowOnlineHelpRequest in v2

nitpick: put the comment above the line.

TylerLeonhardt · 2018-09-18T02:47:19Z

src/features/ShowHelp.ts

+    // I think we can take out the ShowOnlineHelpRequestType... I don't think we actually send it...
+export const ShowHelpRequestType =
+    new RequestType<string, void, void, void>("powerShell/showHelp");
+export class ShowHelpFeature implements IFeature {


nitpick: an empty line between the class and const

TylerLeonhardt · 2018-09-18T02:47:47Z

src/features/ShowHelp.ts

+export const ShowHelpRequestType =
+    new RequestType<string, void, void, void>("powerShell/showHelp");
+export class ShowHelpFeature implements IFeature {
+


nitpick: no newline here :)

TylerLeonhardt · 2018-09-18T02:49:38Z

src/features/ShowHelp.ts

+    private languageClient: LanguageClient;
+
+    constructor() {
+        // TODO: Remove this before merge.


what's this TODO for? To delete the registerCommand?

Yeah, for that commit I wanted to add a way to send the message to PSES, the TODOs are a reminder to remove them before merging the pull request. I mostly wanted it there in case someone wanted to test it to see what it's behavior would be.

TylerLeonhardt · 2018-09-18T02:54:08Z

src/features/ShowHelp.ts

+            }
+
+            const editor = vscode.window.activeTextEditor;
+


nitpick: remove the newline here

TylerLeonhardt · 2018-09-18T02:55:53Z

src/features/ShowHelp.ts

+        this.deprecatedCommand.dispose();
+        // TODO: Remove this dummyCommand too...
+        // Don't forget the one in package.json
+        this.dummyCommand.dispose();


remove this too :)

Dummy command was used for testing PSES.

rjmholt · 2018-09-20T18:52:06Z

@corbob I'm still keen to get this in. If you make those two whitespace edits, we can lock this PR, get the PSES one in and merge this one :)

rjmholt · 2018-09-20T18:49:25Z

src/features/ShowHelp.ts

            const selection = editor.selection;
            const doc = editor.document;
            const cwr = doc.getWordRangeAtPosition(selection.active);
            const text = doc.getText(cwr);

-            this.languageClient.sendRequest(ShowOnlineHelpRequestType, text);
+            this.languageClient.sendRequest(ShowHelpRequestType, text);
+        });


I would put a newline here

rjmholt · 2018-09-20T18:50:01Z

package.json

@@ -88,7 +88,12 @@
      },
      {
        "command": "PowerShell.OnlineHelp",
-        "title": "Get Online Help for Command",
+        "title": "Get Online Help for Command(Deprecated)",


I'd put a space before the opening parenthesis here

rjmholt · 2018-09-20T18:54:07Z

src/features/ShowHelp.ts

@@ -6,34 +6,42 @@ import vscode = require("vscode");
 import { LanguageClient, NotificationType, RequestType } from "vscode-languageclient";
 import { IFeature } from "../feature";

+// TODO: remove ShowOnlineHelpRequest in v2


Do actually need to keep this type around if we only use the new message? We need to keep it in PSES, but maybe not here?

Agreed. AFAICT the ShowOnlineHelpRequestType is not used anymore in this file.

rkeithhill

As soon as the few little nits @rjmholt has pointed out are addressed, this PR LGTM.

Stylistic cleanup.

TylerLeonhardt

Let's do itttttttt 🚢

rjmholt

Awesome work!

Change Menu label: remove Online.

9211bc2

We now look for help locally if it's not availble online. Perhaps to do: remove references to Online from internal commands?

rjmholt reviewed Sep 6, 2018

View reviewed changes

rjmholt requested review from TylerLeonhardt and rkeithhill September 6, 2018 03:48

TylerLeonhardt approved these changes Sep 6, 2018

View reviewed changes

rjmholt approved these changes Sep 6, 2018

View reviewed changes

TylerLeonhardt approved these changes Sep 6, 2018

View reviewed changes

Rename the file to ShowHelp. Continue removing word Online

e0352db

Add a command for PowerShell.ShowHelp and mark OnlineHelp as deprecated. Display warning when OnlineHelp is called and then execute ShowHelp.

TylerLeonhardt reviewed Sep 18, 2018

View reviewed changes

Remove Dummy command. Cleanup structure.

8593ebf

Dummy command was used for testing PSES.

rjmholt reviewed Sep 20, 2018

View reviewed changes

rkeithhill approved these changes Sep 21, 2018

View reviewed changes

Remove showOnlineHelp requestType as it's not used.

9b3ece5

Stylistic cleanup.

TylerLeonhardt approved these changes Sep 21, 2018

View reviewed changes

rjmholt approved these changes Sep 21, 2018

View reviewed changes

rjmholt merged commit ab40a53 into PowerShell:master Sep 21, 2018

corbob deleted the Really-fix-884 branch January 17, 2019 04:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

corbob commented Sep 3, 2018

rjmholt Sep 6, 2018

corbob Sep 6, 2018

rkeithhill Sep 10, 2018

TylerLeonhardt Sep 10, 2018

rjmholt Sep 10, 2018

corbob Sep 11, 2018

rkeithhill Sep 11, 2018 •

edited

Loading

corbob Sep 16, 2018

TylerLeonhardt left a comment

rjmholt commented Sep 6, 2018

rjmholt left a comment

TylerLeonhardt left a comment

TylerLeonhardt left a comment

TylerLeonhardt Sep 18, 2018

TylerLeonhardt Sep 18, 2018

TylerLeonhardt Sep 18, 2018

TylerLeonhardt Sep 18, 2018

TylerLeonhardt Sep 18, 2018

corbob Sep 18, 2018

TylerLeonhardt Sep 18, 2018

TylerLeonhardt Sep 18, 2018

rjmholt commented Sep 20, 2018

rjmholt Sep 20, 2018

rjmholt Sep 20, 2018

rjmholt Sep 20, 2018

rkeithhill Sep 21, 2018

rkeithhill left a comment

TylerLeonhardt left a comment

rjmholt left a comment

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

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

Conversation

corbob commented Sep 3, 2018

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

rjmholt commented Sep 6, 2018

rjmholt left a comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill left a comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

rkeithhill Sep 11, 2018 •

edited

Loading