Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addressing API review feedback #34321

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Addressing API review feedback #34321

merged 1 commit into from
Jul 31, 2024

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Jul 30, 2024

Change ReplacingExpressionVisitor.Replace to IReadOnlyList parameters Make ReaderColumn.GetFieldValueExpression and the new constructor experimental. Make ReaderColumn.Create experimental.
Make ReaderColumn constructor experimental.

Change ReplacingExpressionVisitor.Replace to IReadOnlyList<Expression> parameters
Make ReaderColumn.GetFieldValueExpression and the new constructor experimental.
Make ReaderColumn.Create experimental.
Make ReaderColumn<T> constructor experimental.
@maumar
Copy link
Contributor Author

maumar commented Jul 30, 2024

Did not add old constructors like we initially planned, because having a ctor with Expression<Func<DbDataReader, int[], T>> (new ctor) and Func<DbDataReader, int[], T> (old ctor) results in ambiguous exception when trying to invoke the ctor.

@@ -30,6 +31,7 @@ public abstract class ReaderColumn
/// <param name="name">The name of the column.</param>
/// <param name="property">The property being read if any, null otherwise.</param>
/// <param name="getFieldValueExpression">A lambda expression to get field value for the column from the reader.</param>
[Experimental(EFDiagnostics.PrecompiledQueryExperimental)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should these ctors still be experimental, now that we are not adding the old ones back?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave it experimental, but I don't think it matters too much at this level in the stack.

@maumar maumar merged commit b25cba7 into main Jul 31, 2024
7 checks passed
@maumar maumar deleted the api_changes_take2 branch July 31, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants