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.

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

Added advice basics #54

Merged
merged 3 commits into from
Sep 4, 2018
Merged

Added advice basics #54

merged 3 commits into from
Sep 4, 2018

Conversation

FriedrichWeinmann
Copy link
Contributor

As discussed in the slack channels:

  • Basic tooling to have a launch advice
  • A bare initial set of pieces of advice (need more before going live with that )

@vexx32 vexx32 self-requested a review August 30, 2018 04:05
Copy link
Owner

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

I think there's probably also a more effective and less messy-looking way to split up a string of advice to best suit the console widths...

and we should look at possibly getting the PS Slack and subreddit to chip in and offer their favourite bits and pieces of advice, little tips and so forth.

PSKoans/Data/Advice/Customization/profile.txt Show resolved Hide resolved
@@ -0,0 +1,2 @@
Browsing completion options
When typing a parameter or the value for a parameter, you can hit the TAB key to cycle through the options available. While this is generally well known, it is LESS well known, that by hitting "CTRL"+"SpaceBar" you can show all available options at once and use the cursor keys to navigate to the option you want.
Copy link
Owner

Choose a reason for hiding this comment

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

Auto-complete is also functional for:

  • command names
  • filesystem (PSProvider) paths
  • .NET types and field or property names

All of which should probably be mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? This ain't about what can be completed for - it's about TAB vs. CTRL +SpaceBar, so I picked the most commonly known examples.
Maybe an explicit mention of it working for all situations where tab is used to cycle through options, but that's about as far as I'd go.

Copy link
Owner

Choose a reason for hiding this comment

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

Just seemed odd that you mentioned it works for parameters and their values, but not... everything else PS offers completion for. ^^

The console experience between Linux and Windows is very different. In PowerShell, if you are crossing the boundary, you can still have a lot of the behavior you are used to, using Set-PSReadlineOption:
Set-PSReadlineOption -EditMode Emacs
Set-PSReadlineOption -EditMode Vi
Set-PSReadlineOption -EditMode Windows
Copy link
Owner

Choose a reason for hiding this comment

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

A brief summary of what each of those actually do is probably warranted here, if you can one- or two-line each of those. I'm sure there are some differences between Vi and PS's Vi mode, for example, that ought to be mentioned.


begin
{
$adviceFolder = Join-Path (Join-Path $script:ModuleFolder 'Data') 'Advice'
Copy link
Owner

Choose a reason for hiding this comment

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

Since you're creating the second part of the path, it's best to just do that explicitly, using platform-agnostic slashes (i.e., forward slashes).

Join-Path $script:ModuleFolder 'Data/Advice'

Copy link
Owner

Choose a reason for hiding this comment

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

Also, use parameter names, shame on you! ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a few base cmdlets where yes, I too use positional binding for readability reasons ^^
Had trouble with those forward slashes on some particular windows hosts, so I moved away from them. Probably not relevant for this usecase though.

PSKoans/Public/Invoke-Advice.ps1 Show resolved Hide resolved
{
$adviceFolder = Join-Path (Join-Path $script:ModuleFolder 'Data') 'Advice'

function Write-Line
Copy link
Owner

Choose a reason for hiding this comment

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

This needs a more appropriate name. This is too generic and may be needed by something else at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a function internal command with a single use-case, its name ties perfectly into that foreach loop I'm calling it from.
It would need to be renamed if it became a shared internal function of course.
Maybe to this:?

Write-ConsoleMessage

Would also opt for additional parameteres if we do this:

  • Indent left
  • Indent right
  • Max lines before swallowing text

Wasn't going to bother with the full get-up for a Proof of Concept to show what I had in mind ^^

Copy link
Owner

Choose a reason for hiding this comment

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

That's fair, but I think that's worth doing. :)

}
process
{
$adviceItem = Get-ChildItem $adviceFolder -Recurse | Where-Object PSIsContainer -EQ $false | Where-Object BaseName -Like $Name | Get-Random
Copy link
Owner

Choose a reason for hiding this comment

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

Long line; line breaks please!

{
[string[]]$lines = Get-Content $PROFILE
$lines += 'Invoke-Advice'
$lines | Set-Content $PROFILE
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than bothering to read in the file twice here, you can just use Add-Content to append aline containing the command to the profile script.

I might also be inclined to suggest use of Select-String for the conditional, but it's probably overkill. It would cut out the Get-Content here, though... but not really accomplishing a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point there, didn't think that through properly

Friedrich Weinmann added 2 commits August 30, 2018 09:44
@FriedrichWeinmann
Copy link
Contributor Author

Alright, did a few of the requested changes, commented a lot of the other comments.
One of the thing I'd like you to keep in mind on the advice snippets though:
Overloading them with information will add to total text length, which at some point will cause the user to no longer read them. I'd much prefer to add a link to a guide for more information, in case they want to dig deeper.

Also note: I was doing this in a simple manner in order to show it in action. For a final state I would be looking to:

  • Keep list of shown advice, de-prioritize advice already shown
  • Change advice files to Json, so advice can have properties (Currently in mind: Name, Advice Dependency, Text and Link)

@vexx32
Copy link
Owner

vexx32 commented Aug 30, 2018

Sounds like a good idea. It might be worth linking relevant help topic(s) as well...

@vexx32 vexx32 merged commit 0f41ef1 into vexx32:dev Sep 4, 2018
vexx32 pushed a commit that referenced this pull request Oct 8, 2018
* Added advice basics

* Reformat: Broke pipeline into multiline

For neater display on small editors

* Retionalized appending Invoke-Advice
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.

2 participants