ConstantineK / sqlshell

Lightweight TSQL PowerShell Module
GNU General Public License v2.0
3 stars 1 forks source link

Write-Message needs to be removed and replaced. #3

Closed SirCaptainMitch closed 6 years ago

SirCaptainMitch commented 6 years ago

The functionality for the Write-Message implementation needs to be removed and replaced with something native.

This spans multiple functions and is heavily used. I will update this issue as I continue to do my spike.

Looks like it uses some c# classes to set some defaults.

The primary ones being:

[Sqlcollaborative.Dbatools.dbaSystem.MessageHost] [Sqlcollaborative.Dbatools.dbaSystem.DebugHost]

There doesn't seem to be too much in the way of things that would need to be carried over and a good bit of it is just setting defaults that could be set in the function itself.

The real question will be what do we replace it with?

SirCaptainMitch commented 6 years ago

There are a couple options that we could do to replace the functionality here with something a bit more native to powershell.

Obivously powershell has it's own ways of writing to the host, so we can utilize those, or we can use a proxy function similar to the dbatools implementation only gutted to be powershell native so there is only one call.

SirCaptainMitch commented 6 years ago

The main point would be to keep something like this:

Write-Message -Level Warning -Message "Cannot connect to $SqlInstance"

Which could be useful, but needs to stay Powershell native.

That leaves us with:

  1. Remove the dependency to c# classes, and recreate a similar version of Write-Message to implement. This will make life a little easier in the clean up of the existing functions.

  2. Completely remove all references individually and replace them with Write-Warning/Debug/Host/Verbose depending on the context.

ConstantineK commented 6 years ago

Let me do a bit more digging, because while I am not against a Write-Message (as long as its ps) I thought it was a bit overburdened with both messaging and logging functions and imo we should consider those separately.

In some cases this is cool, set a message level and then if you dont care about the output it can be logged to a central store - but I dont love convention over configuration (even if its one time configuration.) If we could opt in to such a process, that would be sweet.

The other bit is where possible, I think sticking to the native functions that people are and should be familiar with lowers the bar to entry, and doesn't ask the user to learn our conventions and then relearn them in some other project.

That all being said, the only deficit I see in the Write- messages is that they dont all take in the same type of values, some strings, some objects.

If we wanted to make a messaging function that could do the following, I think I would be convinced to let it stay:

I would consider the last bit an advanced user setting, everything works as designed unless you set a special variable or file thing (idk) so that you can get a transcript of all your various outputs (debugging, automation)

mshamann commented 6 years ago

I totally agree with splitting out the Write-Message and Write-Log split. Writing to a log is different that writing messages to the console.

I also agree with standardizing the parameters to take in the same type of values. Imo, it's largely a string and a level (a string.)

I'm not sure I understand this one.

Agreed 100%, this was my intention

I think we can start with redirection, but I agree a Write-Log function should be an option as a switch. Passing Write-Message -Message 'Test' -Level Debug -Log Would be nice. However I think that might be putting the cart before the horse and could be implemented once the Write-Message is finalized.

Ill start working on a prototype of what I am thinking about replacing it with.

ConstantineK commented 6 years ago

Just to clarify on my mumblings on objects and strings:

When using Write-Output or Write-Host you can pass them fully qualified objects and expect some level of pretty printing on the screen, you can even pass them complex expressions which will then be evaluated.

If you do the same thing with Write-Verbose/Debug/Information etc they will choke, they generally want you to send them a nice string representation of what you want.

Being able to send an object or an expression and having it be able to be evaluated and then converted to a string would be wicked sweet.

mshamann commented 6 years ago

Okay, yeah that makes more sense for sure then.

Ill have to play around with that a bit more then. I think on some level we could support it. Although given the way write-debug works. I am not sure.

maybe it recognizes if this is a string vs object?

ConstantineK commented 6 years ago

Yeah that's why I was thinking something like

function Test-String {
  param ($Target)
  if ($Target.GetType().Name -eq 'String'){
    return $true
  } else {
    return $false
  }
}

function Export-String {
  param ($Target)
  if (Test-String -Target $Target){
    return $Target
  } else {
    ConvertTo-Json -InputObject $Target -Depth 10
  }
}

$str = "a string"
$obj = [pscustomobject]@{
  Name = "what"
  Value = "Hello"
}

$StringOutput = Export-String -Target $str
$ObjectOutput = Export-String -Target $obj

$StringOutput.GetType().Name
$ObjectOutput.GetType().Name

and if returns false, cast it to json and then to a string like in

SirCaptainMitch commented 6 years ago

Discussed, going to break this into smaller tickets and close.

  1. Write-message
  2. Write-Log
  3. Config file and supporting functions
  4. Fusion

Write-Message will call a config variable that will tell it if it needs to log or not.

Write-Log will be a split function that will handle output to a log file. $GlobalEnv Config will handle defaults for the module as to avoid system conflicts and messing with the local system when not needed.