Backport Has() function from Serilog.Filters.Expressions

[warrenbuckley]

For users porting over from the deprecated Serilog.Filters.Expressions package. This keeps some feature parity in the template expressions.

This simply adds the Has() function which in turn is calling the new/renamed function of IsDefined()
Adds some tests to ensure the template expression works as expected.

Let me know if you are happy to accept this PR @nblumhardt to fix #64

[nblumhardt]

Thanks @warrenbuckley - I think this is another one that NameResolver is intended to cover.

Thinking some more, if the one small NameResolver could cover both this and the legacy property names, perhaps we could even paste it as a sample into the README here? 🤔

[warrenbuckley]

I have no problem that this is an example in the readme and I can update this PR for that approach.
Would you rather me close this PR and start a new one to show an example in the Readme @nblumhardt ?

Here is my example, however it would be nice if ScalarBoolean was public as it saves me copying code out into this Extension class for the Has() function.

using Serilog.Events;

namespace MyAwesome.Project
{
    public class SerilogExtensions
    {
        static readonly LogEventPropertyValue ConstantTrue = new ScalarValue(true);
        static readonly LogEventPropertyValue ConstantFalse = new ScalarValue(false);

        // This Has() code is the same as the renamed IsDefined()
        public static LogEventPropertyValue? Has(LogEventPropertyValue? value)
        {
            return ScalarBoolean(value != null);
        }

        private static LogEventPropertyValue ScalarBoolean(bool value)
        {
            return value ? ConstantTrue : ConstantFalse;
        }
    }
}

[nblumhardt]

👋

ScalarBoolean is a handy little optimization, but probably not necessary for the example. You can replace it with:

-             return ScalarBoolean(value != null);
+            return new ScalarValue(value != null);

A PR with the example would be great; might not be entirely possible to do, just yet - I'm investigating different options for remapping @m etc.

The logic for producing the message, exception, and so on is not complex, but it's quite a bit of code to reproduce if we try to plug in properties the same way we currently plug in functions. I'm leaning towards proposing something like:

public abstract class NameResolver
{
    public virtual bool TryResolveAlias(string alias, [NotNullWhen(true)] out string? target)
    {
        target = null;
        return false;
    }

where the function would be invoked with both function and built-in property names such as "Has" and "@Message" (but not regular property names), and where the resulting target could be either a built-in or function name in either case.

This would make supporting the legacy syntax fairly straightforward, and still leave some flexibility in there for other use cases/variations.

public class FilterExpressionNameResolver : NameResolver
{
    public override bool TryResolveAlias(string alias, [NotNullWhen(true)] out string? target)
    {
        // Ignoring case-sensitivity..
        switch (alias)
        {
            case "Has":
                target = "IsDefined";
                return true;
            case "@Message":
                target = "@m";
                return true;
            default:
                target = null;
                return false;
        }
    }
}

Any thoughts pro/con?

[warrenbuckley]

Hiya
I will close the other PR and leave this one open for discussion on approach.

But the underlying PR I will send will be an update to the Readme for this, once the approach has been decided

For me I think it would be good to use the two different approaches. So keep the existing Has function and only use the alias mapping for properties only and not functions in my opinion.

To me this would be clearer and avoid confusion for anyone implementing or using this to give a better DX.

As I doubt anyone would be remapping functions with the one exception of Has. But rather than support it for this single use case for backporting.

Let me know what you think and if you have a release or CI build I can use to implement this :)

Cheers
Warren 😀

[warrenbuckley]

@nblumhardt I will close this PR as well now we have #67