Internal types that need to be made public (EFCore.PG, preview7)

[roji]

Here are EFCore.PG's dependencies on internal EF stuff at preview7.

Need to be done:

• RelationalEvaluatableExpressionFilter (for making sure certain methods aren't evaluated locally in funcletization)
• IndentedStringBuilder (when interacting with ExpressionPrinter, printing custom expressions)

Already done in master:

• ExpressionPrinter (for custom expressions)

[ajcvickers]

@roji IndentedStringBuilder we decided not to make public and we reduced the need to use it in 3.0. If it's still too tempting for providers to use, then we could make it real internal. 🚎

ExpressionPrinter is public in current bits. 😸

[roji]

IndentedStringBuilder we decided not to make public and we reduced the need to use it in 3.0. If it's still too tempting for providers to use, then we could make it real internal.

But ExpressionPrinter (which is public) exposes it as its StringBuilder property, and that's the only way to actually print stuff on it :)

[smitpatel]

We can also hide ExpressionPrinter.StringBuilder and provider methods on ExpressionPrinter which would delegate to it internally.

[roji]

Sure, whatever allows providers to generate SQL. I wasn't in the discussions on keeping IndentedStringBuilder internal, but in some cases I suspect expressions would need to be printed multiple lines and hence indents. If we end up exposing significant functionality from IndentedStringBuilder on ExpressionPrinter it may make more sense to just expose it instead... IndentedStringBuilder also doesn't seem to be that big or a large potential for breakage, so I'd just consider making it public.

But no strong feelings on my side, whatever you choose that allows providers to work is fine.

[smitpatel]

I wasn't aware of discussion either. The way I see it, IndentedStringBuilder is implementation detail. If it is being used outside of ExpressionPrinter then sure it could be make public. But if it is exclusively being used for it then it should remain hidden. RelationalCommandBuilder already follows the pattern and only exposes the methods rather than builder itself. If that is not causing any issue then I don't see why ExpressionPrinter should be any different.

[roji]

I don't see a big difference between the two options and have no real preference.

[ajcvickers]

We don't want to make it public because it's a general-purpose useful class that isn't directly related to EF. Historically, people have taken dependencies on EF only to use classes like this, which then results in very different kinds of support requests. It's the same reason a bunch of our extension methods are real-internal.

[roji]

OK then, should I submit a PR to expose the relevant functionality on ExpressionPrinter?

[ajcvickers]

@roji That makes sense to me.

[ajcvickers]

@roji Any updates on this following the move to current bits?

[roji]

Nope - all good. I think we still have to make RelationalEvaluatableExpressionFilter public though - should I do that?

[ajcvickers]

Let's discuss with @smitpatel in triage.

[smitpatel]

Yes, make it public. Providers need to derive from that.

[roji]

Closing as everything covered by this has been made public.