Expose DefaultNameTranslator on INpgsqlTypeMapper#2137
Expose DefaultNameTranslator on INpgsqlTypeMapper#2137austindrenski merged 2 commits intonpgsql:devfrom
DefaultNameTranslator on INpgsqlTypeMapper#2137Conversation
4e77a3c to
fa1a3e6
Compare
fa1a3e6 to
ef9132a
Compare
| [NotNull] | ||
| INpgsqlTypeMapper MapEnum<TEnum>( | ||
| [CanBeNull] string pgName = null, | ||
| [CanBeNull] INpgsqlNameTranslator nameTranslator = null) |
There was a problem hiding this comment.
Doesn't ReSharper understand nullability by default values?
There was a problem hiding this comment.
Unless I'm mistaken, including these annotations here flows them onto implementation classes, which makes it simpler to ensure that concrete implementations all share the same semantics.
It should be obvious to anyone looking at the interface that these can be null, but since it's a public interface, I think it makes sense to add them.
As an example, imagine that an implementation has a different default value for pgName, like pgName = "test_name". With these annotations, ReSharper should still flow nullability info to the caller. But without these annotations, the nullability defaults to unknown (at least historically).
There was a problem hiding this comment.
For god's sake I hope C# 8 comes out soon with nullable references...
YohDeadfall
left a comment
There was a problem hiding this comment.
LGTM, but please address my comment before merging it.
|
@roji This is blocking npgsql/efcore.pg#605 and npgsql/efcore.pg#621 where type mappings need a global fallback translator. Could you take a quick look? |
|
Promise to review this weekend... Sorry, the non-Npgsql workload is really intensive at the moment... |
| internal Dictionary<string, NpgsqlTypeMapping> Mappings { get; set; } | ||
|
|
||
| public INpgsqlNameTranslator DefaultNameTranslator { get; set; } | ||
| public abstract INpgsqlNameTranslator DefaultNameTranslator { get; } |
There was a problem hiding this comment.
I may be missing something, but why not keep this a regular, non-virtual property on TypeMapperBase and make the setter protected? This would save having have two overrides of this property in GlobalTypeMapper and ConnectorTypeMapper...
There was a problem hiding this comment.
Oh, that's a silly oversight. Though, instead of a protected setter, it seems safer to pass this down to a base constructor.
|
Even better! |
|
@roji Since this doesn't seem to warrant being backported to 4.0.3, can we bump the dependency in EF Core provider to the unstable 4.1 build? Its currently pointed at the unstable 4.0.3, but I imagine we plan to release 4.1 alongside 2.2? |
|
@austindrenski I actually don't think we'll be releasing 4.1 alongside 2.2... 2.2 isn't too far off now (preview1 has already been released), whereas we haven't really done much for 4.1. It makes more sense to me to backport this relatively risk-free change to 4.0.3 and take a dependency on that CI version... |
|
@roji In that case I'll backport and retarget those two PRs. |
Supports: npgsql/efcore.pg#621