Skip to content

Handle dependencies better and refactor for reproducibility#57

Closed
Peter J. Mello (RogueScholar) wants to merge 1 commit intoPSModule:mainfrom
RogueScholar:fix-requires
Closed

Handle dependencies better and refactor for reproducibility#57
Peter J. Mello (RogueScholar) wants to merge 1 commit intoPSModule:mainfrom
RogueScholar:fix-requires

Conversation

@RogueScholar
Copy link
Copy Markdown

@RogueScholar Peter J. Mello (RogueScholar) commented Mar 1, 2026

Description

This is a general refactoring seeking several simultaneous outcomes:

  • More tolerant (i.e. sane) and secure dependency resolution
  • Application of implicit environment assumptions as explicit interpreter-managed directives
  • Use of best practices wherever possible, specifically regarding:
    • Grammatical voice for runtime output messages
    • Scripting safety conventions for determinative behavior
    • More performant execution
    • User-friendly process introspection and recovery from failures

Setting a global requirement on PowerShell Core makes the expectation of interpreter expressed in #36 much easier to understand and moves the source of guidance for it from local documentation to the runtime shell.

Shifting from 'RequiresVersion' arguments to 'ModuleVersion' allows newer versions of modules to satisfy dependency constraints and adding the GUID makes the execution of third-party code more secure from namespace clobbering, be it incidental or malicious.

Activating StrictMode at Script scope in PowerShell Core operations is loosely equivalent to the boilerplate set -euo pipefail incantation ubiquitously employed in POSIX shell scripts where potentially damaging activity will occur. Version 3 converts these "code smells" from warnings or recoverable errors to fatal errors:

  • Using method syntax (parentheses and commas) for function calls
  • References to:
    • Uninitialized variables
    • Non-existent object properties
    • Invalid/out-of-bounds collection indexes

The call to create a new GUID has long been an unnecessarily expensive task, and the resulting download path length felt unwieldy, but most concerning was its placement in the user's home directory (${Env:USERPROFILE}, properly, on Windows), where the context is that it is meant to be retained in the event of abrupt execution failures. Much more appropriate is to follow Microsoft's own guidance for ephemeral working directories to reside under User/Machine-scoped ${Env:TEMP} to match the execution scope. This allows them to be cleared away by the built-in Disk Cleanup or any third-party equivalents in the event of unintentional cruft. Adequate randomness now takes the form of the instantaneous Get-Random (insecure implementation) call for a six-digit hexadecimal value that is passed to PSObject.ToString from .NET along with the PadLeft method to ensure the resulting directory name is always of the expected length.

Best comprehension and accuracy for user-consumed runtime messages that announce actions before they are undertaken requires the use of gerund forms or third-person present tense for action verbs. The liberal use of the terminal ellipsis in such strings is a widely-understood rubric that reinforces the nature of the announcements as preemptive rather than conclusory.

Resolves: #53

Type of change

  • 📖 [Docs]
  • 🪲 [Fix]
  • 🩹 [Patch]
  • ⚠️ [Security fix]
  • 🚀 [Feature]
  • 🌟 [Breaking change]

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

This is a general refactoring seeking several simultaneous outcomes:

* More tolerant (i.e. sane) dependency resolution
* Apply implicit environment assumptions explicitly for interpreter to manage
* Use of best practices wherever possible, specifically regarding:
  - Grammatical voice for runtime output messages
  - Scripting safety conventions for determinative behavior

Setting global requirement on PowerShell Core makes the expectation of interpreter expressed in PSModule#36 much easier to understand and moves the source of guidance for it from local documentation to the runtime shell.

Shifting from 'RequiresVersion' arguments to 'ModuleVersion' allows newer versions of modules to satisfy dependency constraints and adding the GUID makes the execution of third-party code more secure from namespace clobbering, be it incidental or malicious.

Activating StrictMode at Script scope in PowerShell Core operations is loosely equivalent to the boilerplate `set -euo pipefail` incantation ubiquitously employed in POSIX shell scripts where potentially damaging activity will occur. Version 3 converts these "code smells" from warnings or recoverable errors to fatal errors:

* Using method syntax (parentheses and commas) for function calls
* References to:
  - Uninitialized variables
  - Non-existent object properties
  - Invalid/out-of-bounds collection indexes

Best comprehension and accuracy for user-consumed runtime messages that announce actions before they are undertaken requires the use of gerund forms or third-person present tense for action verbs. The liberal use of the terminal ellipsis in such strings is a widely-understood rubric that reinforces the nature of the announcements as preemptive rather than conclusory.

Resolves: PSModule#36
Resolves: PSModule#52
@RogueScholar Peter J. Mello (RogueScholar) requested a review from a team as a code owner March 1, 2026 03:03
@MariusStorhaug
Copy link
Copy Markdown
Member

Thank you for the PR. However i will be closing it as it is doing to many disconnected changes, some of which seem unwarranted. I suggest to make issues for the things you mean that are not according to best practice paired with a rationale as to why/what it improves and trade-offs if you are aware of any.

@MariusStorhaug
Copy link
Copy Markdown
Member

Peter J. Mello (@RogueScholar) — the PR has been reviewed and some of the underlying concerns are valid and will be addressed. The following issues have been created to track them individually:

That said, some feedback on the PR itself:

On tone and language. The description characterizes existing, working code with terms like "sane" (implying the current state is not), "code smells," and "unnecessarily expensive" (the New-Guid claim is factually incorrect — it completes in microseconds). Framing personal style preferences as authoritative requirements — such as stating that verbose messages require gerund forms, or that brace-wrapping every variable is a "best practice" — comes across as dismissive of the project's existing conventions. Contributions are welcome, but the language should be collaborative rather than corrective.

On style versus best practice. Wrapping all variables in braces (${var}), rewriting verbose messages to use gerund tense with terminal ellipsis, and swapping New-Guid for a Get-Random hex construction are stylistic choices, not best practices. Presenting them as security fixes or performance improvements overstates their impact and makes it harder to evaluate the genuinely useful parts of the PR.

On PR structure going forward. If there are concerns with the codebase in the future, please open individual issues — one concern per issue, with a clear rationale for why the change matters and any trade-offs involved. This makes it possible to evaluate, discuss, and merge each improvement on its own merits. Omnibus PRs that bundle unrelated changes are difficult to review and often get closed even when they contain good ideas, as happened here.

The valid concerns from this PR are now tracked and will be addressed. Thank you for taking the time to contribute.

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.

Replace RequiredVersion with ModuleVersion in #Requires directives

2 participants