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

Cleanup alias and missing SafeCommands #1745

Merged
merged 13 commits into from
Oct 30, 2020
Merged

Conversation

fflaten
Copy link
Collaborator

@fflaten fflaten commented Oct 28, 2020

PR Summary

  • Cleanup all code in /src/* to avoid usage of alias (related Pester crashes if user does not have alias foreach defined #1740)
  • Adds missing $SafeCommands['xxxx'] on external function-calls in /src/*
  • Adds a PSScriptAnalyzer configuration with initial rules. Should be tuned later if it's going to be used in CI.
  • Adds a custom PSScriptAnalyzer module for build-rules. Only rule atm. is looking for external function-calls that should use $SafeCommands (defined in the dictionary)

PR Checklist

  • PR has meaningful title
  • Summary describes changes
  • PR is ready to be merged
    • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
  • All tests pass
    • Run ./build.ps1 -Clean; ./test.ps1 -NoBuild. Use a new PowerShell process when C# code is changed.
  • [NA] Tests are added/update (if required)
  • [NA] Documentation is updated/added (if required)
    • Should be added for PSSA in the future, but not mandatory or part of CI yet. Rule documented in comment-based help

src/Format.psm1 Outdated Show resolved Hide resolved
src/Format.psm1 Outdated Show resolved Hide resolved
src/functions/Mock.ps1 Outdated Show resolved Hide resolved
src/functions/TestDrive.ps1 Outdated Show resolved Hide resolved
@nohwnd
Copy link
Member

nohwnd commented Oct 29, 2020

Not part of this change, but if you want to come up with a rule that forces us to not use New-Object, Where-Object, Select-Object and ForEach-Object at all it would be nice to have that in a Draft PR. Those cmdlets are much slower than their normal counter parts. I don't have good replacement for some of the usages (as you saw with the Select-Object -First), but this analyzer rule would allow me to find all of them and fix them. We can probably replace Select-Object -First with call to .NET method that uses Linq. New-Object I already replace with Pester Factory where I create types using .NET constructor.

@fflaten
Copy link
Collaborator Author

fflaten commented Oct 29, 2020

After this cleanup, a simple search for e.g. (?:new|foreach|select|where)-object will show all, but a rule could be used for in-editor syntax highlight I guess. Could look into it as a warning rule (no autofix-support). 👍

If support for PSv3 was dropped then .Foreach() and .Where() could've been used. Select-Option needs a custom function-replacement I guess.

Co-authored-by: Jakub Jareš <me@jakubjares.com>
@fflaten
Copy link
Collaborator Author

fflaten commented Oct 30, 2020

sort is mapped to application in unix. Also found that some rules like PSUseSingularNouns don't run in unix, so note to self: Run PSSA in Windows or both.

@nohwnd
Copy link
Member

nohwnd commented Oct 30, 2020

sort is mapped to application in unix.

That is definitely not expected, Sort-Object should be used in all cases.

@nohwnd
Copy link
Member

nohwnd commented Oct 30, 2020

After this cleanup, a simple search for e.g. (?:new|foreach|select|where)-object will show all, but a rule could be used for in-editor syntax highlight I guess. Could look into it as a warning rule (no autofix-support). 👍

No pressure, it was just an idea. I should first get my profiler to work properly and start measuring (if only on PS5.1 because that is the only place where it currently works).

If support for PSv3 was dropped then .Foreach() and .Where() could've been used. Select-Option needs a custom function-replacement I guess.

Having v3 support is important because people still write modules that are compatible down to v3, so they should be able to test them easily.

@fflaten
Copy link
Collaborator Author

fflaten commented Oct 30, 2020

sort is mapped to application in unix.

That is definitely not expected, Sort-Object should be used in all cases.

Yup. Could have given some different results on unix vs windows. Fixed now.

Looks like the CI failure on PSv3 and v4 was caused by me removing one BOM too many in Mock.ps1. PSSA auto-fix added BOM, so had to remove manually, but didn't notice that Mock.ps1 had it originally. Non-consistent use of BOM by design or new issue?

@nohwnd
Copy link
Member

nohwnd commented Oct 30, 2020

Non-consistent use of BOM by design or new issue?

New issue.

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.

None yet

2 participants