Hi @SeeminglyScience could you please review this PR, Thanks in advance!

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@SteveL-MSFT @daxian-dbw @anmenaga @TravisEz13 @SeeminglyScience Apologies for tag SPAM, but can I please get this PR reviewed? It had no reviewer activity for 2 weeks and it's blocking some other things I would like to work on.

@kilasuit I will try to rebase it onto trunk, hopefully won't be too difficult despite 3 years of changes.

I don't think wrapping the entire PR in an experimental flag makes any sense, as this is primarily a bug fix. If you really want me to stick an experimental flag somewhere, I can make the $PSStyle customization experimental, and hardcode current values otherwise.

That sounds like a good plan to me.
Get the rebase done and push that up first and then if you can go thru the the experimental feature process to do I can make the $PSStyle customization experimental that sounds good to me

@kilasuit Surprisingly, the merge went fine, all conflicts resolved automatically.

I am having a trouble making the experimental feature though, the [Experimental(ExperimentalFeature.PSStylePromptFormatting, ExperimentAction.Show)] does not appear to work on non-cmdlet classes.

I am basically trying to achieve the equivalent of the following:

public sealed class PSStyle {

    [Experimental(ExperimentalFeature.PSStylePromptFormatting, ExperimentAction.Define)]
    public PromptFormatting Prompt { get; }

};

static PromptFormatting GetPromptFormatting()
    => ExperimentalFeature.IsEnabled(ExperimentalFeature.PSStylePromptFormatting)
    ? PSStyle.Instance.Prompt
    : new PromptFormatting();

So that if the experimental feature is not enabled, the $PSStyle.Prompt does not exist at all, an the prompt formatting uses default-constructed class for hardcoded defaults.

I am not sure this is at all possible with the current tooling, looking at how PSAnsiRenderingFileInfo experiment was implemented, the $PSStyle.FileInfo always existed, it was just ignored if the experiment was not enabled. I could do the same using the GetPromptFormatting() above, but I think it makes little sense.

Also, it might be that I can no longer trigger the automated checks due to not having contributed in a long time, as I can see "6 workflows awaiting approval" above the checks.

@iSazonov can you kick off the workflows for this please?

@daxian-dbw Thanks for recheck, the only one failing seems to be some category requirement?

Error: Every PR must have at least one label starting with 'cl-'.

Looking at other PRs suggests to me that it needs some maintainer tagging it as CL-General, or maybe CL-Engine.

The default color settings need to be reviewed.

My intention was always to have that reviewed by someone from the accessibility team. I like your example of only using uncolored and bold text plus yellow, should be quite good baseline as it's already used in warnings/errors.

Sorry that it's been delayed for so long for us to get back to this PR.

Given the issues I found above, I believe we need to do full review of the PR again before moving forward. Once the review on code changes is done, the interactive console WG needs to review the default colors.

@iSazonov and @SeeminglyScience can you please do a full review of this PR? Thank you!

@iSazonov and @SeeminglyScience can you please do a full review of this PR? Thank you!

I've already started...

@daxian-dbw Looking how we used PSStyle.OutputRendering and SupportsVirtualTerminal (including NOCOLOR) previously I think we need more convenient/generic method than specific method like GetFormatStyleString(FormatStyle formatStyle). I'd think about refactoring PSStyle so that it uses properties which dynamically evaluate and return values based on OutputRendering and SupportsVirtualTerminal.
(Otherwise, we'll get very confusing and poorly maintained code here. We will be doomed to do amazing things - first construct strings, then analyze and parse them, and clean them in StringDecorated.)

@iSazonov Absolutely agree, that was at least partially what those TODOs are about. I can open a new issue once this PR gets merged.

I'd prefer to refactor PSStyle first of all since this will add more capabilities and simplicity to current PR.

@iSazonov Normally I would agree that refactor enabling fix comes first. But a 3 year old PR is not normal.

@iSazonov Normally I would agree that refactor enabling fix comes first. But a 3 year old PR is not normal.

This only means that the MSFT team lack resources and the activity of the community is too low.

Without improving of PSStyle you'll have to do the same job here, and then we'll have to move it to PSStyle.

This only means that the MSFT team lack resources and the activity of the community is too low.

My own activity was pretty high before I got stuck in this echo chamber pinging people who ignored me, so I moved to other repos. That's my last comment on that, since it's not something either of us can fix here.

Without improving of PSStyle you'll have to do the same job here, and then we'll have to move it to PSStyle.

I would rather do it this way around, so we get the fix in quickly, then I can refactor at my leisure. Frankly I don't remember most of the code I wrote here and would need more time getting back to it. If it doesn't get merged before end of the year, I'll just close it since apparently the bug doesn't bother anyone else.

It doesn't honor $PSStyle.OutputRendering or NO_COLOR environment variable. When setting $PSStyle.OutputRendering = 'PlainText' (or setting the No_COLOR env var), the default colors for multi-choice prompt are still applied

Looking how we used PSStyle.OutputRendering and SupportsVirtualTerminal (including NOCOLOR) previously I think we need more convenient/generic method than specific method like GetFormatStyleString(FormatStyle formatStyle).

@daxian-dbw I think the cleanest way would be to add PSStyle.Plain with all the props set to empty strings. That way the caller can do something like var style = shouldUseColors() ? PSStyle.Instance : PSStyle.Plain; and keep other code as-is. Might also add this directly into PSStyle as GetActiveInstance(). though baking it directly into PSStyle.Instance is likely to be a bad idea, since it's supposed to be a simple getter with no heavy logic.

But, because the old implementation did not honor this setting either, I think it should be handled as a separate issue.

@dkaszews thank you for the patience with this and willingness in coming back to it after so much time. I can only from the community side of the I-UX WG, apologise that this has been so long in getting to this point. I know this apology will be echo'd from those on the Microsoft side too. To set expectations we aren't meeting again this year either, but that doesn't mean this pr will stick much longer, but I'm not sure it'll get merged in prior to the new year. If you do close the PR, there's always the chance to cherry pick commits, which would still see you getting the credit for the work you've done so far with this PR

Normally I would agree that refactor enabling fix comes first. But a 3 year old PR is not normal.

Unfortunatly whilst this is far from ideal, or typical, there are many reasons why PR's like this one can be in a state of limbo. Again I can only apologise that this one has been so long to do so.

I agree on everything you said on in your last 2 comments 2 points particularly

Without improving of PSStyle you'll have to do the same job here, and then we'll have to move it to PSStyle.

I would rather do it this way around, so we get the fix in quickly, then I can refactor at my leisure.

There's plenty more PSStyle work that we'd like to see happen too so this could be added into the bucket of work around that area, I particularly would like to see

• Consider moving the $ErrorView configuration to $PSStyle #17917
• $PSStyle.Formatting.Information #16932
• Make $PSStyle extensible for users and module authors #20482
• Consider easier to read file lengths in directory listings.  #24011

fixed (ideally without me/others asking Copilot to do so) & some of these don't really have lots of work needed to fix them either but is a definitely a project to tackle this list for in the new year.

I think the cleanest way would be to add PSStyle.Plain with all the props set to empty strings

baking it directly into PSStyle.Instance is likely to be a bad idea, since it's supposed to be a simple getter with no heavy logic.

But, because the old implementation did not honor this setting either, I think it should be handled as a separate issue.

Agree with those 3 and as such again should be a new issue to manage.

I don't think I can ask Copilot to review this (perhaps due to lack of access or it being unable to do so)

That said I asked copilot.microsoft.com to have a quick look and it left these comments that may be of use @dkaszews but you'd likely get these yourself from Github Copliot if you use it.

@kilasuit Thank you for your kind words, I appreciate your attention continuing to support this. My attitude towards any Microsoft products is really strained, but working for a top IT company myself, I know it is important not to miss human for a cog in the machine.

That said I asked copilot.microsoft.com to have a quick look and it left these comments

Unfortunately, I find it quite telling that in lieu of any actionable feedback or next steps that could bring the PR to a close, you paste company-mandated LLM slop. You appear to not even notice the "I don’t have the actual diff yet" in very first sentence, likely caused by the PR being older than the LLM bubble itself.

I particularly would like to see [...] fixed (ideally without me/others asking Copilot to do so)

Furthermore, even mere consideration that LLMs can be used to create PRs while those crafted by human contributors go ignored for literal years, truly makes the "quite" in "quite insulting" completely redundant, though not at all surprising seeing Microsoft's recent track record.

If you do close the PR, there's always the chance to cherry pick commits

Feel free, I would like this work to not go wasted, though seeing complete lack of interest I find it quite unlikely for someone to pick it up in the future.