Expose Fake.TryGetFakeManager and Fake.IsFake

[asgerhallas]

I wonder if it would be possible to make Fake.TryGetFakeManager public?

I have a test utility that makes auto-wrapping fakes and I would like to ensure that the provided object is not already a fake - preferably without catching an exception :)

[blairconrad]

Hi, @asgerhallas. Thanks for suggesting this.

My first reactions:

1. why not wrap things that are already fakes?
2. it seems a little strange to not already know whether you have a fake, but then again, I don't know much about your utility
3. I don't really see any harm in exposing the method, truth be told, so why not?

Counterpoints, @thomaslevesque?

[thomaslevesque]

I don't see any harm either in exposing it either, but maybe not in its current form. Currently it returns the FakeManager directly, or null if it's not a fake, so it doesn't follow the standard pattern for Try* methods. We might want to change it to something like this:

bool TryGetFakeManager(out FakeManager manager)

Alternatively, we could expose a IsFake method that should be called to ensure GetFakeManager will succeed. It's probably more natural to call a method named IsFake to check if the object is a fake, anyway.

[thomaslevesque]

In fact, we could expose both IsFake and TryGetFakeManager

[blairconrad]

Yeah, I was considering resignaturing the method as well. I… guess it's the right thing to do, even though the nullability of the return value (I'm skipping ahead a bit) would serve as well to indicate that there's no result.
I mean the whole bool TryXXX(out ResultType) signature only exists because of a lack of good Maybe class anyhow. And where we know that a trueish result will not be null and a falseish result will be null, it seems like extra ceremony.

I think it comes down to whether we want to continue to support these established patterns or help establish new ones. Even though I think the usual signature is slightly antiquated, it's probably best to conform at this point?

I've no objection to exposing IsFake.
(Although I will point out that IsFake + GetFakeManager combine to replace TryGetFakeManager…)

[asgerhallas]

Hi @blairconrad and @thomaslevesque,

Per your points @blairconrad:

1. why not wrap things that are already fakes?

Just feels like a little too much indirection. I guess at some point I will get an error message, I won't understand, related to this :)

2. it seems a little strange to not already know whether you have a fake, but then again, I don't know much about your utility

The utility stores fakes for later retrieval (in memory, in a list, very simple). And I want to allow users of the utility to configure the fakes e.g:

Store(A.Fake<IInterface>(x => x.SetupALot())) 

But also just fall back to defaults:

Store<IInterface>(new InterfaceImpl()) // which just wraps the provided class in a fake

It seems minor, but when used in quite large test suite it means a lot.

3. Yes, why not? :D

I agree very much with this:

I mean the whole bool TryXXX(out ResultType) signature only exists because of a lack of good Maybe class anyhow. And where we know that a trueish result will not be null and a falseish result will be null, it seems like extra ceremony.

Just wish they'd made a `if (x.TryGet() is not null value)´ pattern matching thing, though I guess I could do somethng like this i C# 8:

var result = Fake.TryGetFakeManager(fake) switch {
    null => A.Fake<T>(),
    _ => fake
    };

So I'm all for "help establish new ones". Like FakeItEasy has done before :)

[thomaslevesque]

I think it comes down to whether we want to continue to support these established patterns or help establish new ones. Even though I think the usual signature is slightly antiquated, it's probably best to conform at this point?

Well, we're not going to establish new patterns on our own, I think... I used to dislike the TryXXX pattern, but now with out variable declarations, it's not so bad. I rather like it now.

Return null means the caller has to check for null. Using TryXXX, that's not necessary. Compare:

// existing TryGetValue method
if (dict.TryGetValue(key, out var value))
{
      ...
}

// vs hypothetical TryGetValue method returning null if not found
var value = dict.TryGetValue(key);
if (value != null)
{
    ...
}

Of course, it's also possible to use if (dict.TryGetValue() is TheType value), but I'm ambivalent about this, because it looks like we're checking the type, when we're really checking if the value is null...

(Although I will point out that IsFake + GetFakeManager combine to replace TryGetFakeManager…)

Yes, but it's inefficient... two steps instead of one. More code, and more work for the computer.

Just wish they'd made a `if (x.TryGet() is not null value)´ pattern matching thing

You can use if (x.TryGet() is TheType value), as mentioned above

[blairconrad]

Okay, @thomaslevesque. I'm convinced. On Fake,

public static bool TryGetFakeManager(object potentialFake, out FakeManager fakeManager);
public static bool IsFake(object potentialFake);

(If this gets added after nullability warnings are enabled, I'd mark the fakeManager parameter in the first call with a NotNullWhen(true).)

Should be fairly straightforward, even including updating the Advanced Usage docs.

[asgerhallas]

You can use if (x.TryGet() is TheType value), as mentioned above

Yes, but...

it looks like we're checking the type, when we're really checking if the value is null...

That was the point I tried to get to with my switch and all. Sorry, I wasn't clear 🙂

Well, we're not going to establish new patterns on our own, I think... I used to dislike the TryXXX pattern, but now with out variable declarations, it's not so bad. I rather like it now.

Hmm... yeah, you've got a point. Composing is a tiny (really tiny) bit more work though:

// existing TryGetValue method
return dict.TryGetValue(key, out var value)) ? value : A.Fake<IInterface>();

// vs hypothetical TryGetValue method returning null if not found
return dict.TryGetValue(key) ?? A.Fake<IInterface>();

Therefore I tend to choose the "new pattern" and rely on the Try-prefix to warn me that this could return null. I might change my opinion in the future though... I often do 🙂

But really not a real problem anyway. I'm just happy that you want to expose the functionality 🤗

[blairconrad]

Great, @asgerhallas.

I'm just happy that you want to expose the functionality

The next question is whether you want to expose the functionality. The issue is near and dear to you. Are you interested in implementing it?

[asgerhallas]

Hi @blairconrad, sure! :)

Is this usable #1711?

[blairconrad]

Closed by #1712.

[afakebot]

This change has been released as part of FakeItEasy 5.5.0.

[blairconrad]

Thanks for proposing and implementing this issue, @asgerhallas. Look for your name in the release notes! 🥇