dataplat / dbatools

🚀 SQL Server automation and instance migrations have never been safer, faster or freer
https://dbatools.io
MIT License
2.39k stars 787 forks source link

New-DbaDbUser: Refactoring and bugfixing #9386

Closed andreasjordan closed 3 days ago

andreasjordan commented 3 weeks ago

Type of Change

As I have to (re-)refactor this command a second time after the first refactoring in 2022, I want to do something new: Have a command with a lot of comments to tell other contributors how and why we do things.

I changed Stop-Function to support -WhatIf. Without that change, the variable is not set and Test-FunctionInterrupt does not work. Then, if the command fails in the begin block, the process block is still executed.

As I'm not a native english speaker, please correct all my mistakes in the new comments.

And there are some "Discussion" comments - which I would like to discuss.

andreasjordan commented 3 weeks ago

Thanks @niphlod for the feedback, just fixed some typos.

One additional point: Pipeline Input. A lot of commands accept pipeline input, most other ...-DbaDb... commands accept database SMOs to be piped in. That would also be my recommendation. But I wanted to get feedback before changing the code.

niphlod commented 3 weeks ago

on "the pipeline" ... I guess it has not been "declared" a sound/strict rationale behind it. Prolly the author inserts it the way "it was envisioned" in the first place or it has been added as an additional request. I don't think we should "force" to have all the commands targeting dbs to accept a pipelined "list of smo dbs", nor "force" login-related parameters to accept a "list of smo logins", etc etc etc. tl;dr: I see it as a "nicety" (granted it makes sense for some kind of useful behaviour) rather than an "obligation" to adhere to.

potatoqualitee commented 2 weeks ago

will merge once approved 👍🏼

potatoqualitee commented 3 days ago

Hell yeah, love that approval. Thank you both so much 🙇🏼