The main idea is that due to limitations of method overload resolution we cannot have [...]Should(IReadOnlyDictionary<TKey,TValue>[...]. It will always conflict with overload accepting IDictionary.

You mean that the compiler doesn't accept a Should(this IDictionary) and Should(this IReadOnlyDicitonary)?

@dennisdoomen the compiler does support having both overloads.
IDictionary doesn't inherit from IReadOnlyDictionary, but Dictionary implements both interfaces, so the conflict is that Dictionary.Should() is ambiguous between Should(this IDictionary) and Should(this IReadOnlyDictionary).

Frankly, I don't like a ShouldReadOnlyDictionary extension method. With version 5, I've went to great lengths to make sure all APIs are consistent.

I think I understand your point. It is in a sense a scratch on the glass.

However, I dislike the fact that I do not have all dictionary assertions on IReadOnlyDictionary more. I stumbled upon it today yet again when testing some Serilog properties that are exposed using that interface.

I think that there is a decision to make. (A) We can go practical on this one and agree to some form of impurity (whether we will choose ShouldReadOnlyDictionary or some other name does not matter that much). (B) We could also decide to abandon assertions on IReadOnlyDictionary and just close related issues.

@jnyrup @dennisdoomen I would appreciate a clear decision on that matter, thanks.

What about replacing the overload for IDictionary with IReadOnlyDictionary? Both Dictionary and ReadOnlyDictionary implement that interface?

hat about replacing the overload for IDictionary with IReadOnlyDictionary? Both Dictionary and ReadOnlyDictionary implement that interface?

Clever idea, I will try it out in the evening.

Well, it worked suprisingly well. Please review the changes (ignoring GenericReadOnlyDictionaryAssertionSpecs.cs and friends, they will be removed).

Just few tests had to be adjusted, other than that it worked like a charm.

The only test failing is When_the_execution_time_of_a_member_is_close_to_a_limit_it_should_not_throw. I guess it needs to be dealt with on master.

Moreover, I have just cleaned up classes from initial ShouldReadOnlyDictionary implementation.

From a unit test perspective I like the change, as the assertions shouldn't use any methods not available from IReadOnlyDictionary.

But this would be both an ABI and API breaking change.

• Own custom dictionaries only (currently) implementing IDictionary<TKey, TValue>
• Third party libraries not compiled for .NET 4.5+ (The first edition of the IReadOnly interfaces), where it's harder/impossible to add IReadOnlyDictionary<TKey, TValue>.

While the potential problems mentioned above are most likely only the case for a minority, my gut feeling is still that such a breaking change should be postponed to a major release.

I have to agree with @jnyrup. Unless we can think of a way to prevent the breaking change, we'll have to postpone this to a future major release.

Planning the feature for the next major release is not a problem at all. I will have time to polish the implementation and think about downsides.

Thanks for mentioning the fact that IReadOnlyDictionary is not present before 4.5. I was not aware.

I know there's been lots of talk how to support this, however, I think one way to solve it would be to have the dictionary methods on an IEnumerable<KeyValyePair<TKey, TValue>>:

public static class DictionaryExtension
{
    public static void Example<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> value)
    {
        if (value is IDictionary<TKey, TValue> dictionary)
        {
            UseDictionary(dictionary);
        }
        else if (value is IReadOnlyDictionary<TKey, TValue> readOnlyDictionary)
        {
            UseReadOnlyDictionary(readOnlyDictionary);
        }
        else
        {
            throw new NotSupportedException("value must be a dictionary type");
        }
    }

    private static void UseDictionary<TKey, TValue>(IDictionary<TKey, TValue> dictionary)
    {
    }

    private static void UseReadOnlyDictionary<TKey, TValue>(IReadOnlyDictionary<TKey, TValue> readOnlyDictionary)
    {
    }
}

The following works fine with the above:

new Dictionary<string, int>().Example();

The above works as both interfaces (IDictionary and IReadOnlyDictionary) implement IEnumerable<KeyValuePair<TKey, TValue>>. Since we've limited it to KeyValuePair, a normal List<string> won't be applicable for the extension.

However, the above would match a List<KeyValuePair<string, int>> - I personally have never seen that used in production code though (and especially now we have named tuples in C# 7, I think it's useage would be extremely rare). To solve that we could perhaps provide another extension method on the diciontary assertions to cater for the rare chance someone is using it (maybe, AsCollection?) that would only be applicable to IEnumerable<KeyValuePair<TKey,TValue> to help move them back to the collection assertions. It might be a case though of waiting for the error to be reported, as I think it will be far more rarer than the use of IReadOnlyDictionary

Happy to have a stab at another PR if you prefer to avoid confusion with this one.

I'm late for this but here's my two cents.

The conflict happens not only for IDictionary/IReadOnlyDictionary but also for ICollection/IReadOnlyCollection and IList/IReadOnlyList. Although FluentAssertions implements assertions for ICollection, IList and IDictionary, it only uses the enumeration part of these interfaces, which is read-only. The assertions should take the read-only interfaces as parameter and not the read-write counterparts.

Still, these assertions are incomplete. They test only one form of enumerating the collection. For example, a collection that implements IReadOnlyList<T> can be enumerated using the indexer, using IEnumerable<T>.GetEnumerator() or using IEnumerable.GetEnumerator(). There's no guarantee that they are all correctly implemented. The Count property can also return the wrong value.

The collection may not even implement any interface or may have a public GetEnumerator() that is not an overloaded interface.

This is fine if you assume the iterators are well implemented but not if you're implementing an iterator. That's my case so I implemented a BeExactlyAs() extension to FluentAssertions: https://github.com/NetFabric/NetFabric.Hyperlinq/blob/419762f44f54f67f2ff312585cea3db8cabeb987/NetFabric.Hyperlinq.UnitTests/Utils/FluentAssertions/Collections/ReadOnlyListAssertions.cs

It would be great is this feature was added to the API.

Now that we're accepting breaking changes again, I think it's time to reconsider this PR.

Hi again, I managed to merge with master and fix all functional tests successfully but some approval tests are failing.

I have yet to learn what they are and how to fix them :-). Could you point me to some resources? Locally build.ps1 runs successfully.

Ok, I merged with develop and fixed the approval tests. I changed *.approved.txt files since the API really does change for this PR.

What is important is that I had to disable one test which did not compile with the new API. It was the test that used a dictionary that did not implement IReadOnlyDictionary. It would be a breaking change. The tradeoff is:

• we don't support dictionary assertions of dictionaries not implementing IReadOnlyDictionary
• instead we do support dictionary assertions on IReadOnlyDictionary objects, which is arguably more important and much more common

Do any more discussion/decisions/doubts come to your mind?

What we could do is to provide an AsReadOnlyDictionary extension method that converts the IDictionary into a IReadOnlyDictionary for those folks that really need to work with an IDictionary and for which the GenericCollectionAssertions don't provide the value they want.

I think I adjusted the PR according to all your comments.
Tomorrow, for completeness, I will write some tests for the extension method.

Anything else to complete the task?
Maybe changelog or docs?

BTW: I wrote simple Powershell script approving new API, maybe you will find it useful:

Remove-Item *.approved.txt*
Get-ChildItem -Filter "*received.txt" | ForEach-Object {
	$NewName = $_.FullName -replace 'received.txt', 'approved.txt'
	Copy-Item $_.FullName $NewName
}

Maybe changelog or docs?

Yeah, good thinking. We wanted to start updating https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md for anything on develop. And updating the docs needs to happen as well.

BTW: I wrote simple Powershell script approving new API, maybe you will find it useful:

Yep. Please add it to the root of the repo. Something like AcceptApiChanges.ps1. Maybe good to add some remarks at the top of the file to explain what the script is supposed to do for contributors.

I have just added an entry in the releases.md and some clarification in dictionaries.md.
LGTM I guess.

I merged with the develop branch but test When_the_execution_time_of_a_member_is_close_to_a_limit_it_should_not_throw failed to time related reason. Is there any way to trigger re-build?

@jnyrup is this one still on your radar?