Operation 'Leaving things better than we found them'

[jpomfret]

Hey folks,
This discussion has stemmed off of this comment that @NowinskiK added to the standards - I don't agree so wanted to open the floor to discuss.

The standards are a living doc, as we find things to standardise we'll add to it and then there will be some rework to retrofit these standards to the current code.

However!

I think we should require new commits follow these guidelines otherwise we will forever be chasing our tails. I like to live by a motto that if I open a function or some code to add a feature or fix a bug, I will read the rest of the code and see if there are things to fix - at the moment this should include

• swapping to use PSFramework - it makes logging so much easier, and gives people a lot of options when using the module (like logging to file, graylog, etc.)
• removing return - this is a super easy fix when we're opening code files we can address as we go

When dbatools took a 'short code freeze' to fix all these kinds of issues it took us literally YEARS ha - we are early enough that we can catch this now and start moving from good towards great.

Please, discuss :)

[NowinskiK]

I like the name of the operation, @jpomfret 😊, I really do! I will wait with my comments for others first.

Also just to explain: I added the box on "Standards" to highlight sections we are not following yet, because in PR we have mandatory checkbox which requires contributors to validate their changes.

[jpomfret]

No need to wait @NowinskiK - looking for others opinions, that's the point of the discussion 😄

Yeah - this is what I'm disagreeing with - I think we should be following these from now.

Also just to explain: I added the box on "Standards" to highlight sections we are not following yet, because in PR we have mandatory checkbox which requires contributors to validate their changes.

[tiagobalabuch]

@jpomfret you're absolutely right on this. Chasing our own tail is the last thing we want, and setting a solid foundation now will make everything easier in the long run. I’d suggest we align on three key items first so we're all pulling in the same direction:

1. Adopt PSFramework – This is a big change, but I’m fully on board. After diving into it, I completely agree that it’s the right move. Huge thanks to you and Rob for bringing it to our attention!
2. Splatting – I'm in favor of updating our internal code with splatting. My only suggestion is to provide examples that show different usage styles, like single-line versions or those using backticks, to help with clarity.
3. Return functions – Totally agree with your and Rob’s points here. It makes sense and it’s definitely the simplest of the three.

Getting alignment on these will give us a strong, shared foundation to build on.

My proposal: Let's hit pause on creating new functions until we’ve cleaned things up. I know Kamil is excited to contribute more (which is awesome!), but if we build on a shaky or inconsistent base, we risk running into bigger problems later like what Jess mentioned about the early days of dbatools. Let’s learn from that experience and do it right the first time.

Looking forward to working through this together and building something great as a team!

[NowinskiK]

I agree with all these changes as well (some are documented in Standards already). The only thing I ask is to document the approach with examples and agree before we start developing them.

1. Adopt PSFramework - Use PSFramework for logging #84
2. Splatting - Code formatting: Splatting input parameters before Invoke-FabricRestMethod calls #87
3. Return functions - Use `return` in functions or not - that's the question #134 (I discussed with Tiago about this and believe we can move forward)

[jpomfret]

Yeah agree with those three for sure - PSFramework will be a big change - but adds a lot of power as well both to us within the module, and others that will use the module.

[tiagobalabuch]

@jpomfret @NowinskiK I believe we're all aligned on the three points, right?

The only thing left to clarify is who will be responsible for providing the examples in the documentation?

[NowinskiK]

Yes, you are right! Thanks for your feedback here @tiagobalabuch , thanks for understanding and patience with us @jpomfret .
I see consensus for each of the issues, hence I moved them from Backlog to Ready (status).

Documentation - this is part of code changes.
Unless you meant "Standards" (wiki documentation) - it can be a joint effort as it has been so far. If we have different opinions, they should be discussed on the issue or discussion item first, and that's it.
For the existing items discussed above - each task should have corresponding changes in Wiki (if needed) made by a developer who is making the changes in code for the first time.