[API Suggestion]: DataTableCollection and DataColumnCollection ContainItemWithName

[logiclrd]

The DataTableCollection and DataColumnCollection types in System.Data have items with logical names. An API enhancement is proposed to allow succinct expression of assertions that these collections contain items with provided names. This functionality was originally implemented within pull request #1812, but has been separated off for independent consideration in pull request #1893.

Specific API surface changes proposed:

public static class DataTableCollectionAssertionExtensions
{
    public static AndConstraint<GenericCollectionAssertions<DataTable>> ContainTableWithName(this GenericCollectionAssertions<DataTable> assertion, string expectedTableName, string because = "", params object[] becauseArgs) { }
    public static AndConstraint<GenericCollectionAssertions<DataTable>> NotContainTableWithName(this GenericCollectionAssertions<DataTable> assertion, string unexpectedTableName, string because = "", params object[] becauseArgs) { }
}

public static class DataColumnCollectionAssertionExtensions
{
    public static AndConstraint<GenericCollectionAssertions<DataColumn>> ContainColumnWithName(this GenericCollectionAssertions<DataColumn> assertion, string expectedColumnName, string because = "", params object[] becauseArgs) { }
    public static AndConstraint<GenericCollectionAssertions<DataColumn>> NotContainColumnWithName(this GenericCollectionAssertions<DataColumn> assertion, string unexpectedColumnName, string because = "", params object[] becauseArgs) { }
}

[jnyrup]

What would the proposed syntax offer over using the existing Contain(Expression<Func<T, bool>>)?

dataSet.Tables.Should().Contain(e => e.TableName == expectedTableName);

[logiclrd]

Most significantly, DataTableCollection and DataColumnCollection actually have specialized Contains methods that interact (presumably) with the underlying collection, rather than just enumerating it.

https://docs.microsoft.com/en-us/dotnet/api/system.data.datatablecollection.contains?view=net-6.0#system-data-datatablecollection-contains(system-string)

https://docs.microsoft.com/en-us/dotnet/api/system.data.datacolumncollection.contains?view=net-6.0

I haven't checked but I suspect that the underlying storage keeps the elements sorted and these methods use a binary search.

[jnyrup]

So you'd like an assertion that utilizes DataTableCollection.Contains to mimic however it interacts with DataTableCollection and its potential speedups.

I had a quick glimpse at DataTableCollection.Contains(string name).
It does a linear scan over the tables plus some logic how to compare tables with same name but different casing or namespace.

[logiclrd]

Huh. Well, that's disappointing, but also I feel that that's its own internal implementation detail decision to make. It's breaking boundaries to say, "Well, they do that internally, so we can just do the same thing externally." The presence of a Contains public method really makes me want to call that method instead of implementing the same linear search myself. I do understand that in practice it's not winning anything in this case, due to this wrinkle.

[jnyrup]

I also notice that DataSetAssertions<TDataSet>.HaveTable(string expectedTableName) calls DataTableCollection.Contains.
Couldn't you call

dataset.Should().HaveTable(expectedTable);

instead of the proposed?

dataset.Tables.Should().ContainTableWithName(expectedTable)?

[logiclrd]

That's fair. I guess it isn't going to be terribly common for people to encounter a DataTableCollection without it being a part of a known DataSet.

[logiclrd]

I created these for parity with other collection assertions, but it wouldn't be unreasonable to argue that the likelihood of encountering the collection without its containing type is vanishingly small, and the containing type can already do equivalent assertions.

[jnyrup]

We should not be adding an API only because it can be implemented.
For every API we should be asking ourselves whether it's going be used in practice and what benefits it has over any existing API.
I have fallen into the trap of adding an API just because it was easy to implement, despite it not being broadly useful.
With todays eyes I would most likely have rejected my own PR #604 as too niche.

It's not a goal for us for System.Data to have parity with the other collection assertions.
As I hear you say, one should in practice always be able to call dataset.Should().HaveTable(expectedTable) in a test.
Unless I'm missing something, I believe we can close this with a rejection of the proposed API?

[logiclrd]

I think that's so, yeah.

[dennisdoomen]

I agree with @jnyrup here

[jnyrup]

That settles it then.
Thanks for the discussion.