KEMBAR78
Add internal methods to check Preferences by iSazonov · Pull Request #25514 · PowerShell/PowerShell · GitHub
Skip to content

Add internal methods to check Preferences #25514

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

Jump to bottom
Merged
merged 2 commits into from
May 7, 2025
Merged

Add internal methods to check Preferences #25514

merged 2 commits into from
May 7, 2025
+30 −0

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented May 6, 2025

PR Summary

Add internal methods to check Preferences. It allows to check if output enabled before doing expensive work.

It is implemented only in MshCommandRuntime class with true defaults for other implementations of ICommandRuntime2/ICommandRuntime interfaces since extending of interfaces is changing public API. Although this can now be done using default interface implementations (without breaking changes), new public APIs require approval by the working group and the committee, as well as changes to the SDK package. All this can be done later after the PR.

if (cmdlet.IsWriteDebugEnabled()
{
    string dbg = <expensive work to create debug string>;
    WriteDebug(dbg);
}

PR Context

PR Checklist

@iSazonov iSazonov requested a review from SeeminglyScience May 6, 2025 09:21
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label May 6, 2025
@iSazonov iSazonov mentioned this pull request May 6, 2025
Merged
12 tasks
@JustinGrote
Copy link
Contributor

JustinGrote commented May 6, 2025 •
edited
Loading

Looks good to my amateur eyes. I would think we would need test cases for all the various places variable preference can be set beyond just the -verbose and -debug switches, including things like cross-module scopes, etc.

I feel like an ideal version of this would be a WriteDebug overload that takes a delegate etc. and simply doesn't do that execution, but the time for that was 15 years ago, so this is the next best option that is simple to utilize. I think the caveat is though anything external that wants to use this is going to need a preprocessor directive or its going to only work on 7.6+, I don't think we can really polyfill in extension members for this once c# 14 is availalble.

@iSazonov
Copy link
Collaborator Author

iSazonov commented May 7, 2025

@JustinGrote The current status is that these are just internal helper methods. This is the first step. Later, we can discuss whether they are needed publicly. As for the tests, they will be required if these methods become public. Now they are just internal helpers and do not change the existing code.

SeeminglyScience
SeeminglyScience approved these changes May 7, 2025
View reviewed changes
Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

Looks good to me!

FWIW my two cents is that these should stay internal. I do think there is room for conditionally generated messages in a public API, but for that I would prefer it be surfaced in form of message factory delegates (and maybe overloads for format strings, but that's probably over kill).

(I realized all of that is sort of off topic for this PR, just getting my thoughts out).

Thanks Ilya!

@iSazonov

This comment was marked as outdated.

Sign in to view
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@iSazonov iSazonov self-assigned this May 7, 2025
@iSazonov iSazonov merged commit 7d6b6d8 into PowerShell:master May 7, 2025
37 checks passed
@iSazonov
Copy link
Collaborator Author

iSazonov commented May 7, 2025

@SeeminglyScience Thanks for fast review!

@microsoft-github-policy-service
Copy link
Contributor

microsoft-github-policy-service bot commented May 7, 2025 •
edited by unfurl-links bot
Loading

📣 Hey @@iSazonov, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

@iSazonov iSazonov deleted the is-debug branch May 7, 2025 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SeeminglyScience SeeminglyScience SeeminglyScience approved these changes

Assignees

@iSazonov iSazonov

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iSazonov @JustinGrote @SeeminglyScience