Skip to content

Auto-complete comment help for functions #748

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

Merged
merged 12 commits into from
May 18, 2017
Merged

Conversation

@kapilmb
Copy link
Author

kapilmb commented May 15, 2017

Here is quick demo of the feature -

auto-help-completion

@daviwil
Copy link
Contributor

daviwil commented May 15, 2017

That is incredibly cool! Does the ## form of help work for PowerShell's help system?

@daviwil
Copy link
Contributor

daviwil commented May 15, 2017

It'd be nice if we could remove the selection of the text after it gets inserted. Even better if the cursor could be placed inside of the synopsis field (though I know it's probably not super easy). In the future it'd be nice to do this as a dynamic snippet so that you can tab through the fields to enter the new text.

@rkeithhill
Copy link
Contributor

In the future it'd be nice to do this as a dynamic snippet

That would be cool and could replace the current comment-help snippet.

@kapilmb
Copy link
Author

kapilmb commented May 15, 2017

Yes, ## works with PS help system.

I think removing the selection and putting the cursor in the synopsis field shouldn't be hard as we know the location of the synopsis relative to the trigger characters. But, I think tabbing through the fields without going through the snippet completion provider might be difficult. I am not sure if snippet provider has a default completion mode i.e. the user doesn't have to tab through the completion candidates, the editor just inserts the best match (a la I am feeling luck). If it has such a feature then we can perhaps remove a lot of the custom logic from the client side.

@daviwil
Copy link
Contributor

daviwil commented May 15, 2017

You can provide default values for the snippet fields:

A snippet can define tab stops and placeholders with $1, $2 and ${3:foo}. $0 defines the final tab stop, it defaults to the end of the snippet. Variables are defined with $name and ${name:default value}. The full snippet syntax is documented here.

The user can confirm the defaults by hitting ESC I think. It looks like you can insert a snippet into the document without a completion provider, check out the TextEditor.insertSnippet method:

insertSnippet(snippet: SnippetString, location?: Position | Range | Position[] | Range[], options?: {undoStopAfter: boolean, undoStopBefore: boolean}): Thenable<boolean>

Might be doable! Only real challenge is inserting the snippet syntax into PSScriptAnalyzer's correction without it making the code nasty.

@kapilmb
Copy link
Author

kapilmb commented May 15, 2017

wow! stay tuned for the snippet version then! 😃

@daviwil
Copy link
Contributor

daviwil commented May 15, 2017

Awesome! I think that'd make it pretty much perfect :)

@rkeithhill
Copy link
Contributor

We will probably want to remove the current comment-help snippet. Also, some folks use <# for generic multiline comments or to comment out sections of code. Just want to make sure we don't get comment help in these scenarios.

@daviwil
Copy link
Contributor

daviwil commented May 15, 2017

I think this functionality only appears if you type <# at the top of a function definition, or at least that should be the case.

@rkeithhill
Copy link
Contributor

But what if my intent is to comment out the function? :-) In general, you can avoid snippet completion by pressing Esc or clicking away.

@daviwil
Copy link
Contributor

daviwil commented May 15, 2017

In this case you'll get some really nice comment-based help to go with your commented-out function ;) We'll polish it up after the first iteration.

@kapilmb
Copy link
Author

kapilmb commented May 15, 2017

How about this -

auto-help-completion-tab-stops

@daviwil
Copy link
Contributor

daviwil commented May 16, 2017

Perfect! Does it work to cancel out of the property fields when you hit ESC?

@kapilmb
Copy link
Author

kapilmb commented May 16, 2017

We will definitely polish the experience, but here is another workaround for the case when if someone just wants use <# without the comment help, they can just undo the completion (ctrl-z or whatever key they have assigned to undo) which will give you the original <# without any help.

@kapilmb
Copy link
Author

kapilmb commented May 16, 2017

By cancel out the property fields do you mean that the tabbing loses focus?

@rkeithhill
Copy link
Contributor

rkeithhill commented May 16, 2017

Looking good. My only qualm is the ###### form of comments. In the years since we've had comment-based help, I can't say I've ever seen that form of help commenting. Do you we want to promote that style?

@kapilmb
Copy link
Author

kapilmb commented May 16, 2017

I think we should leave it up to the user to choose between BlockComment or LineComment style. But maybe by default we should set the comment style to BlockComment ?

@rkeithhill
Copy link
Contributor

rkeithhill commented May 16, 2017

If we allow LineComment style, someone's going to ask to configure the length of the first and last ############# lines. :-)

@daviwil
Copy link
Contributor

daviwil commented May 16, 2017

I also don't see the #### style very often (or ever, to be honest) so I'm not sure how many people would even try it. Maybe it's worth only completing the <# style in VS Code at first when the user is typing and then wait to see if anyone asks for the other form. If they do, we could easily turn it on.

As far as the PSScriptAnalyzer rule is concerned, I'm fine with the user having an option there so that part of the code could definitely stay in.

@kapilmb
Copy link
Author

kapilmb commented May 16, 2017

If we allow LineComment, someone's going to ask to configure the length of the first and last ############# lines. :-)

Good point. May be we just remove the ###### at the beginning and the end - It will still leave the comment help valid and we don't have deal with issue of its length. I have seen people use the line comment style but as both of you have mentioned, it is rare. I will just leave the option there in PSSA, since it is already there, but it doesn't have to be present in the extension.

@rkeithhill
Copy link
Contributor

I will just leave the option there in PSSA, since it is already there, but it doesn't have to be present in the extension.

Sounds good to me.

@daviwil daviwil added this to the 1.1.0 milestone May 17, 2017
@daviwil daviwil changed the base branch from develop to master May 17, 2017 22:40
@kapilmb kapilmb changed the title [WIP] Auto-complete comment help for functions Auto-complete comment help for functions May 18, 2017
@daviwil daviwil merged commit 396286e into master May 18, 2017
@daviwil daviwil deleted the kapilmb/help-completion branch May 18, 2017 04:31
@andysimmons
Copy link

Love it! This is awesome.

@brsh
Copy link

brsh commented May 18, 2017

How about if the snippet could work INSIDE the function? (a la https://technet.microsoft.com/en-us/library/hh500719.aspx: 'It’s preferable to include the help within the function rather than just before. That helps keep the comment "part" of the function, and ensures Windows PowerShell won’t confuse it for something else.').

Plus, I like it better :)

@rkeithhill
Copy link
Contributor

rkeithhill commented May 18, 2017

@brsh Not everybody agrees with that placement (I sure don't). :-) That said, seems like it could be made to work in both locations. You probably prefer tabs over spaces and cuddled elses too, eh? ;-) Generally folks are going to have their "religious" viewpoint on how their code should be styled. And we try to accommodate.

@daviwil
Copy link
Contributor

daviwil commented May 18, 2017

We're going to make it work both ways. PSScriptAnalyzer's rule which provides this behavior is capable of placing it both outside and inside of the function definition, we just need to expose it as an option in the extension. I've just filed issue #763 to track this for the next feature update.

@stefanstranger
Copy link
Contributor

I agree with @brsh I also like to have my help within the function so when I collapse functions the help is also collapsed within the function. The Function Snippet in the ISE with help outside the function bothered me so much I created new Snippets to solve this ;-)
But it is for sure a personal thing :-)

@snd3r
Copy link

snd3r commented May 19, 2017

Awesome guys!
What about cmdlet comment help based on [CmdletBinding()]?

@daviwil
Copy link
Contributor

daviwil commented May 19, 2017

@rbobot you mean above CmdletBinding in a function definition or somewhere else? If in a function, #763 is tracking that.

@brsh
Copy link

brsh commented May 19, 2017

Woohoo! Thanks! I'm all for personal opinions (and for sometimes changing them: I'm coming around on spaces vs tabs), but it's great to have tools that can support them (and people who support those tools). This extension is what moved me over to vscode. It just keeps getting better, and I'm grateful!

@snd3r
Copy link

snd3r commented May 29, 2017

@daviwil I mean CmdletBinding in a file definition (ps1, psm1).

@Scuzz3y
Copy link

Scuzz3y commented May 29, 2017

The only issue with this is that to compress multi-line comments there has to be a space on each line of the multi-line comment. More of a bug with the extension I'd say though.

@jkavanagh58
Copy link

@rbobot Are you referring to having this comment based auto-complete help for a script, not just limited to functions? That is my question anyway.

@snd3r
Copy link

snd3r commented Jun 1, 2017

@jkavanagh58, I always wrote comment based help for each script file and it would be convenient for me if this method worked not only for functions but also for the entire script.

@jkavanagh58
Copy link

jkavanagh58 commented Jun 1, 2017

@rbobot agreed! I know this issue is "Auto-complete comment help for functions" but wondering why it would be limited to functions.

@daviwil
Copy link
Contributor

daviwil commented Jun 1, 2017

Hey guys, here's the issue tracking that feature request: #769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants