ansible-windows
ansible-windows copied to clipboard
Various fixes, optimizations and code formatting
Hello,
please see commit messages for details. Also, this is my first pull-request in public repository, so I'll appreciate any critique and suggestions about contributing workflow in general and your repo in particular.
Thanks for taking your time to work on this I see you've put in quite a bit of work. I'm not sure if I would accept this PR as it is because it makes a lot of stylistic changes when it isn't needed. While I understand some of the changes you've made conform more to the standard set in PowerShell these style guides are never set in stone. Always be mindful of the choices the original developer has made when creating a pull request.
There still seems to be a good few improvements like | Out-Null
to > $null
and the temp path handling change. Feel free to modify or open a new PR with those changes in there. Some of the things I think you should avoid changing in another person's PR are;
- Variable casing, I typically follow
snake_case
compared tocamelCase
orPascalCase
. IMO neither one is right or wrong, the key is to stick with one standard - Quotes from
"
to'
, technically"
can be more dangerous when it comes to escaping variables but I see no real difference to change existing quotes to'
when"
has worked just fine - Function parameters from inline to
param ()
blocks are definitely more PowerShell like but for internal commands it really isn't that big of a deal and just make unecessary noise in the PR diff - Changing the
$null
comparisons to-not $var
may work but I like to be explicit. Although I did make the mistake of putting$null
on the right hand side of the operator
Feel free to disagree with my points here, I did make this script when I first started with PowerShell so it was more aligned with how I worked in other languages.
In saying all this, the beauty of open source is that it allows you to fork the code and make your own changes and do whatever you want with it. There's nothing stopping you from making whatever changes you desire and adding features that are specific for yourself.
Thank you for your feedback, I really appreciate it.
Definitely code style changes should be discussed thoroughly beforehand, so I'll rework this pull request. I still have some questions, though:
- Mostly you wrote about code style and that is related to 2a070a80397fa066e26849bac1ae7f01ec907b0f and, partially, to 40258aaace8e6b1fd0a57893da1d9ab2f6ecf7d7, but what about other commits, which make more functional changes in script's logic? Should I include them into the next pull request?
- Is it better in that case for me to rebase commits (delete the unwanted and amend in code style of the desired), or revert unwanted commits to preserve history?
Mostly you wrote about code style and that is related to 2a070a8 and, partially, to 40258aa, but what about other commits, which make more functional changes in script's logic? Should I include them into the next pull request?
I would like to add the functional changes you've made as they seem like good additions but would like to separate those changes from the stylistic changes. Once we have the functional stuff in then I probably could entertain some of the stylistic changes but that's a better discussion for a separate PR.
Is it better in that case for me to rebase commits (delete the unwanted and amend in code style of the desired), or revert unwanted commits to preserve history?
Really entirely up to you. When I go to merge the PR I would probably just squash and merge unless keeping the history is valuable.
ОК, it has taken some time, but finally I made it. Definitely next time I'll discuss stylistic changes beforehand , rather than editing commits and resolving conflicts. Would be a lesson, though. But I was surprised that git squashed conflicted commits without asking after I resolved them while rebasing interactively. I hope it wouldn't be a problem.
Dear @jborean93, are you there? Let's finish improving your script by accepting the suggested changes. Please take a moment to do this.