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

Multiple code formatting best practices, and some bug fixes combined #114

Merged
merged 7 commits into from
Aug 14, 2022

Conversation

BZanten
Copy link
Contributor

@BZanten BZanten commented Aug 10, 2022

Hi all,
I love the project, and would like to contribute to it.
When I started to look into the code I saw a lot of small formatting inconsequences, and a number of best practices for PowerShell Code are not followed.
This strikes me, as the script itself is meant to help us implement 'security best practices' so I'd recon the script itself then should also comply to PS Code best practices.

So I let the PSScriptAnalyzer run on the code, both for formatting, and best practices.
This led to the following changes. Most of them are minor formatting changes, but also 2 bugs were solved using the output from Script Analyzer

1 Removed trailing spaces using VSCode + Trailing Spaces addon
2 Updated Smart quotes “ and ” with straight quotes "
3 Updated codeformatting via PSScriptAnalyzer: Invoke-Formatter -ScriptDefinition (Get-Content .\Invoke-HardeningKitty.ps1 -Raw) -Settings CodeFormattingOTBS
4 Bugfix replaced = with -eq
5 Replaced Invoke-Expression with PS splatting to solve PSScriptAnalyzer warning regarding PSAvoidUsingInvokeExpression
6 Resolved all instances of PSAvoidUsingPositionalParameters by adding the parameter names.
BugFixed the call to Out-IniFile by removing the additional unused parameter
7 Switch types in Functions advanced parameters have a value of False set by default, and should not be set to false during declaration. Also bugfixed the .IsPresent usage (because being present doesn't mean it is true or false).
See https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_functions_advanced_parameters

Invoke-ScriptAnalyzer -Path .\Invoke-HardeningKitty.ps1
Now only gives a number of debatable warnings, we can look into them later.

I do realize lots and lots of lines are now updated with these updates. So I hope there are no other code changes pending, they'd cause large amounts of merge conflicts. But we can always discuss to re-do my changes at another time. most of the changes were automated and all can be re-done in an evening (as I just did ;-)

Regards, and hope this helps.

Note I DO have lots of PowerShell code experience, but hardly any 'GIT' experience, so if my propositions should be sent-in another way, that is totally fine with me.

…Definition (Get-Content .\Invoke-HardeningKitty.ps1 -Raw) -Settings CodeFormattingOTBS
…r warning regarding PSAvoidUsingInvokeExpression
…the parameter names.

BugFixed the call to Out-IniFile by removing the additional unused parameter
…by default, and should not be set to $False during declaration. Also bugfixed the .IsPresent usage (because being present doesn't mean it is true or false).

See https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_functions_advanced_parameters
@0x6d69636b
Copy link
Owner

First of all, thank you very much! I will look into it and run various function tests. However, I won‘t have time this week, I will post further comments as soon as possible

@BZanten
Copy link
Contributor Author

BZanten commented Aug 11, 2022

No problem, I have time enough, I'm on holiday anyway ;-)

If I can help, would gladly do so. If you need any more info on some of my proposed changes, let me know.

I also would like to propose some future features like creating it into a 'real module' ( .PSD1 / .PSM1 file) so it can be nicely posted on PSGallery.. but that can be discussed later.

@0x6d69636b 0x6d69636b self-assigned this Aug 14, 2022
Copy link
Owner

@0x6d69636b 0x6d69636b 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, thank you for your this help! 👍

@0x6d69636b 0x6d69636b merged commit 50343d2 into 0x6d69636b:master Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants