Skip to content

2025-03 Update #32

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

Closed
wants to merge 3 commits into from
Closed

Conversation

JonathanPitre
Copy link

@JonathanPitre JonathanPitre commented Mar 4, 2025

Well, this update is long overdue! I have integrated existing pull requests, added changes from the dev branch, and fixed a few bugs.

I'm using Cursor AI with Claude with some neat PSScriptAnalyzer rules and powershell linting to work as efficiently as possible. If you are wondering why there's so many changes, most of them are formatting related or error checking.

I've tested this several times on my local machines and it works flawlessly.

  • Fixed VMIntegrationService error on non-English systems #24
  • Fixed authentication error to use MgGraph #29
  • Added error checking when AutopilotConfigurationFile.json is missing
  • Added fix when multiple Autopilot profile exists
  • Improved code formatting
  • Improved comments
  • Fixed errors when using custom Virtual Machines Disks and Configs path
  • Added dynamic memory support for VM creation
  • Improved path handling for ISO and VM locations
  • Added support for internal virtual switch configuration
  • Improved error handling for ISO path validation

Captain Firmware added 2 commits March 4, 2025 17:42
…es#24

Fixed authentication error to use MgGraph tabs-not-spaces#29
Added error checking when AutopilotConfigurationFile.json is missing
Added fix when multiple Autopilot profile exists
Improved code formatting
Improved comments
Fixed errors when using custom Virtual Machines Disks and Configs path
Added dynamic memory support for VM creation
Improved path handling for ISO and VM locations
Added support for internal virtual switch configuration
Improved error handling for ISO path validation
@tabs-not-spaces
Copy link
Owner

Adding AI generated formatting changes in with functionality improvements makes this close to unreviewable.

Changing small things like ! to -not provides zero benefit to anything and obfuscates the actual improvements to the module.

Updating the readme to put your name in the collaborators section is not your responsibility.

I will review every change proposed and make adjustments where necessary.

@JonathanPitre
Copy link
Author

JonathanPitre commented Mar 5, 2025

-Not instead of ! makes it easier to read for non-programmers, hence the PSScriptAnalyzer rule about it. You may not agree, but that's a best practice, and this is what AI is based on.

Also, the formatting was set to OBTS (One True Brace Style) for a reason, you can read more about it here.

I added my name since I spent two weeks fixing this code for free and fixed most of the bugs, and I figured I would save you some time. I think I deserve some credit. Ultimately, you have to approve the change anyway. I honestly don't care, I just want to see the module updated and working for a change.

I'm not trying to be a pain in the ass here but dude, this module is great, and this update is long overdue, and we both know many people asked for it. Let's move forward, I suggest you test the code as is to start with to validate any potential issue.

Oh, btw, you're welcome.

@IISResetMe
Copy link

Also, the formatting was set to OBTS (One True Brace Style) for a reason

What is the reason exactly?

OTBS' cuddled else/catch preference makes it an absolute nightmare to read IMO, and the linked blog post even acknowledges this:

When I look at the OTBS example above, my mind initially just sees one big block of text, rather than 4 distinct parts (function, foreach, if, else). Maybe it’s because I’ve been using Allman for so long, but the compactness of OTBS makes it feel cluttered to me.

(emphasis added)

@JonathanPitre
Copy link
Author

JonathanPitre commented Mar 5, 2025

I have noticed on several occasions that If Else statement would break unless formatted in OBTS when running code directly from VSCode.

PowerShell/vscode-powershell#3087

@tabs-not-spaces
Copy link
Owner

I think I deserve some credit.

That is where contribution comes in. If this PR is reviewed and approved, your name will be attached to the repository. The readme document outlines the initial collaborators who helped design this project.

Also, the formatting was set to OBTS

I will not approve styling changes on a largely abandoned module that was written over 5 years ago.

I also strongly oppose any pipeline configuration changes without issues being raised to identify known problems. Without existing issues, changes to pipeline configs are an immediate red flag.

At the end of the day, this is a community tool written primarily for research and educational purposes. Contributions are appreciated, but submitting PRs with such significant changes and style and formatting changes adds time and effort on my behalf to review this.

If you want to take another stab at this with JUST the proposed fixes and none of the AI formatting fixes (undoing hashtable formatting, renaming variables for no particular reason, changing double quotes to single quotes), I'm happy to review it.

Remember the cardinal rule to successful PRs - small, simple-to-read changes that resolve specific Issues will always be appreciated and approved faster than re-writes.

For now, I will be closing this PR. Thankyou for your understanding.

@JonathanPitre
Copy link
Author

Fine, we will agree to disagree. This code will eventually be published under a new module name once I tweak a few more things.

The variables that were renamed were renamed because they didn't make sense anymore like "win10" for instance or had typos in them.

As it stands AI writes better code than 70% of what's available on GitHub and 100% better code than abandoned project. 😁

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.

3 participants