Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

API for exposing the Schema Column from DbDataReader #5609

Closed
wants to merge 1 commit into from
Closed

API for exposing the Schema Column from DbDataReader #5609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/System.Data.Common/src/System.Data.Common.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
</PropertyGroup>
<ItemGroup>
<Compile Include="System\Data\Common\DbColumn.cs" />
<Compile Include="System\Data\DbSchemaFactory.cs" />
<Compile Include="System\DBNull.cs" />
<Compile Include="System\Data\CommandBehavior.cs" />
<Compile Include="System\Data\CommandType.cs" />
Expand Down
12 changes: 12 additions & 0 deletions src/System.Data.Common/src/System/Data/DbSchemaFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;

namespace System.Data.Common
{
public abstract class DbSchemaFactory
{
abstract public List<DbColumn> GetColumnSchema(DbDataReader dataReader);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the parameter be named simply reader?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should this be List<T>? Has this been API reviewed?

The framework design guidelines say not to use List<T> in public APIs... Usually the recommendation is Collection<T> or ReadOnlyCollection<T> (or a subclass of one of those). In this case, it seems like you want some kind of read-only collection at the very least, because it doesn't seem like a caller of GetColumnSchema should be modifying the collection.

(Though, I am torn about this guideline as List<T> is more efficient than Collection<T> and ReadOnlyCollection<T>. Perhaps the guidelines should be reconsidered with an eye towards potentially allowing the use of immutable collections in APIs? Edit: Indeed Roslyn and System.Reflection.Metadata components expose ImmutableArray<T> in public APIs, so there is some precedent.)

cc: @terrajobst @KrzysztofCwalina

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of type (I agree with ReadOnlyCollection<T> here): abstract public => public abstract for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.
@justinvp and @NickCraver Thanks for the feedback.

@justinvp I have not done a formal API review yet. I do intend to send out a request when submitting a Contracts change. I did want to expose the idea/direction to @NickCraver using code to get any feedback from him and answer questions if any.

I will look forward for any comments from @terrajobst @KrzysztofCwalina
I will send a review based on the documentation at https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

}
}